Skip to content
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

function/stdlib/keys: accept marked arguments #94

Merged
merged 1 commit into from Apr 20, 2021

Conversation

mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Apr 19, 2021

Two changes:

  • Add AllowMarked: true

  • Re-apply top-level marks to result
    If the entire map is marked, those marks are applied to the resulting list of keys. Inner marks are not preserved (only values can have marks).

@mildwonkey mildwonkey force-pushed the mildwonkey/sensitive branch 4 times, most recently from 5a45c63 to 62cea37 Compare April 20, 2021 12:39
@mildwonkey
Copy link
Contributor Author

tests are passing at the moment but seriously do not take my word for it; I seem to be having A Morning

Since map keys can never be marked themselves, and this function doesn't
return any of the values, we can safely ignore any markings on element
values and just translate the shallow map marks to the result list as a
whole.
@apparentlymart
Copy link
Collaborator

Thanks @mildwonkey!

I concur with @alisdair's comment about the UnmarkDeep call; it doesn't hurt to have it there because this function doesn't consider the values of the map elements anyway, but it also wasn't contributing anything to the result because it can only make changes to the map values.

The rest of this looks just as I'd expect though, so I've just rebased this onto latest main and removed that UnmarkDeep call just to avoid the cost of rebuilding another copy of the map when we don't strictly need to. I'm going to merge this now. Thanks again!

@apparentlymart apparentlymart merged commit 470dd69 into zclconf:main Apr 20, 2021
@mildwonkey mildwonkey deleted the mildwonkey/sensitive branch April 20, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants