Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enhancement(remap): Add join function #6313
enhancement(remap): Add join function #6313
Changes from all commits
9f85c36
d7037ec
7c7b91c
3bdd7dd
d0cabfe
af81b9f
7de0384
ed4b560
667f551
0792925
543f716
ae7d5ae
2fbb6fe
9d9048d
b2148f3
29b3c2b
88b3eb9
4f3a098
71153e7
287e8fc
35d0bfe
650c9d5
222a532
0d146e7
3cf898d
5d600ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to this constraint, but the naming / implementation feels a bit awkward to me. This might be due to the fact that
inner_type
is a bit awkward right now in-general, but, based on the current naming, I'm wondering if this function shouldn't be settingself.fallible = true
if a non-array type is passed into it.Another suggestion would be to call this
fallible_unless_inner_type(...)
and implement it such that it could accept any of our types that has an inner type.In my dream world, I think maybe it'd make sense to have
Kind::Array
itself be an enum type so that you could do things like:Kind::Array(Kind::Integer | Kind::Boolean)
to represent an array of integers/booleans, but that would be a more substantive refactoring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jszwedko Good call on that. I've updated the behavior to set
fallible
totrue
for cases when there's no inner type (plus a test for that 😎). I do like your suggestion about the desired end state but I'll leave that to one of our Remap Hero(in)es.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could be made much more generic. Some of the different rules we could want:
["a", "b", "c"]
or[{message: "a"}, {message: "b"}, {message: "c"}]
I think we should wait to see if we actually ever need that level of flexibility before tackling it because it would take a fair bit of thought to get right. So for now I'd say it's good enough to have a fairly specific function that does what we need, but no more. It does result in the awkward name, but I think that reflects the specific nature of where we are up to so far!
I share this vision! The issue here is that
Kind
is a bit flag, soKind::Array | Kind::String
is still aKind
. Which is nice. If we makeKind:Array
an an enum type, it could no longer be a bit flag. We would then need to distinguish between primitive Kinds and complex Kinds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that seems to be the distinction here 👍