-
Notifications
You must be signed in to change notification settings - Fork 90
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
Improve code documentation, second batch #106
Conversation
//! - *Values*: merging any other values succeeds if and only if these two values are equals, in which case it evaluates to | ||
//! this common value. | ||
//! | ||
//! Note that merging of lists is not yet implemented. |
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 suppose that, on list, merging ought to be “the lists have the same size, and we recursively merge corresponding elements”. Or maybe not, I don't know. Is there a design issue?
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.
Not really, it's just that lists were added after merging and this particular point has not been discussed. I think we can either not give it a special treatment (only merge identical lists), and rely on a future support for user-defined merge to do something different, or implement directly your suggestion.
//! - *Default/default*: merging two terms with default (either `Default` or `ContractDefault`) fails, as there cannot | ||
//! be two default values for the same field |
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.
Shouldn't it be the merge of both default values, really?
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.
Yes, probably. Let me make an issue and fix it in another PR.
b43108f
to
19b0a39
Compare
Follow-up of #105.