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
Add Enum Map Derive Macro #279
Add Enum Map Derive Macro #279
Changes from 11 commits
16225b6
0fc2a6b
0ab13a4
8e488b2
8e7aa05
3ccbb3c
7bd40a3
6112e6d
b66a612
8f63bdf
510a160
6a20524
e4ec66c
c3eed1c
5f2f1c1
69da326
f283ab2
5d65bfc
7a16006
d68b5ba
c4d4ca5
f6ed414
5df0666
b965277
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.
Having a variant that could be disabled and cause a panic if indexed with makes me nervous, but it seems to be in line with what this crate normally does. I feel like it would make more sense, though, to ignore the disabled attribute for this specific macro (among others, like
AsRefStr
) since generating code for the ignored attributes can't cause any harm.If they already weren't going to index into this map with that variant (e.g. they were using
EnumIter
with a disabled variant to iterate through all the variants and modify thisEnumMap
with each variant), then there's no harm in not panicking if they try to index with what would be a 'disabled' variant. It could cause confusing behavior if they accidentally do index into this with the disabled variant (since it would basically do nothing instead of panicking), but I feel like that's much better behavior than panicking. Perhaps this comment should be made into an issue instead to discuss there.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.
I think that unexpected behavior would be more detrimental to a project than a panic, since the panic is obvious what the problem is
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.
I agree with this decision - calling this a
map
implies that every possible variant has its own value and if you have a variant that has data (e.g.Name(&'static str)
) it could potentially be confusing if indexing with that variant but different data (e.g.map[Name("John")]
vsmap[Name("Jane")]
) returns/modifies the same value.However, this doesn't exactly seem to go in line with what this crate normally does - e.g. with
EnumIter
, it just generatesdefault::Default()
for each field if a variant has data (instead of refusing to expand the macro). If we wanted to keep behavior similar to the other derive macros here, we might want to still expand the macro but just ignore all data on each variant and only match on variant type. I can't think of a specific use-case where allowing data would be necessary, so I'm still in favor of keeping this as-is, but I'm not quite certain what other contributors would be in favor of.