Skip to content
This repository has been archived by the owner on Oct 4, 2020. It is now read-only.

StrMap.fromRecord #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewleon
Copy link
Contributor

create a StrMap from a homogeneous record

@hdgarrood
Copy link
Contributor

My only concern is that the runtime representation of these two types might not necessarily coincide in other backends.

@matthewleon
Copy link
Contributor Author

@hdgarrood StrMap is JS FFI only, no?

@matthewleon
Copy link
Contributor Author

Perhaps the solution is to add a level of indirection before the unsafeCoerce?

@hdgarrood
Copy link
Contributor

How would a level of indirection help?

Currently I think alternate backend maintainers have been forking the core libraries and translating the FFI code; just because there's only JS FFI stuff here, I don't think that means that we can ignore the issue of alternate backends. I think I'd prefer to address #118 before merging this. Other than that, though, this seems great. See also #125

@matthewleon
Copy link
Contributor Author

@hdgarrood I've made a change, above, that I think should address the concern by moving the coercion to FFI code. Happy to wait for resolution to #118 .

@hdgarrood
Copy link
Contributor

hdgarrood commented Dec 16, 2017

Ohh, I see what you mean now. Yeah, that does seem like it should address that concern. Do you think you can work out how to make the linter happy?

@matthewleon
Copy link
Contributor Author

The linter loves me now.

@hdgarrood
Copy link
Contributor

Great! Before merging I'd like to get approval from someone else, as I'm not sure I'm fully aware of the implications of adding this function. I'd also like to wait until 0.12 to merge this, because changing dependencies of core libraries is generally something that we need to take a bit of care with and has potential to break things, and so it's probably best to wait until we're breaking everything anyway.

@@ -235,6 +236,9 @@ fromFoldableWith f l = pureST (do
for_ l (\(Tuple k v) -> runFn4 _lookupST v (f v) k s >>= SM.poke s k)
pure s)

-- | Create a map from a homogeneous record (all attributes have the same type).
foreign import fromRecord :: forall r t. Homogeneous r t => Record r -> StrMap t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea for foreign functions to take constraints. It's better to declare an "unsafe" impl function for internal use, and add a constraint to the PS wrapper. Honestly, I wish PS altogether disallowed constraint syntax in foreign imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will fix.

create a StrMap from a homogeneous record
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants