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.
Design document for variable mutability and namespacing #469
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
Design document for variable mutability and namespacing #469
Changes from all commits
0d91a2d
296c87b
da944d0
aeb1df9
595bb0b
3cb687b
11f85b0
e7cda16
152b73c
24c9c73
2c81fb0
5376884
0bbf851
da848ad
fb3da4c
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.
I don't think this should be considered as a requirement. At best it's a nice-to-have feature improving static analysis when a single translated message needs to be considered in isolation, and not with respect to its form in the source locale.
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.
Would you prefer a use case for this? This specific list was taken from @stam's comment in #310 near the end (after I had written a longer and more detailed set of requirements). The "static analysis" requirement is kind of "boiling down" the question of whether people will make mistakes with overwriting.
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 looks an awful lot like we're doing pass-by-reference for formatting function arguments and we're modifying the
$external
value in the calling context. Is that intentional?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.
It intentionally looks like it is modifying the value. In practice, it (probably) does not have write access to the external value and is only masking it for the duration of this message. But the idea is that the value is mutable only if you are consciously mutating it.
Note that we could disallow
modify
to externals, that is, you have to eitherlet
ormodify
a local variable in order to annotate an external var.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.
The proposed design has this same flaw:
Anyone not working with MF2 daily will easily forget when looking at that message which prefix is for external and which for internal ones, leading to the exact same failure mode.
Is this the only argument against this alternative?
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.
The proposed design has the matching example in the next block, in which I call out the similarity error. But it isn't "the same flaw". In this case, the declaration completely blocks the original value.
This may not be super terrible. Not having separate namespaces is conceptually simpler and the ability to just annotate via declaration is much easier than doing multiple assignments and teaching developers/translators/etc. about local vs. external vars. Maybe
modify
(however we choose to spell it in the end) solves the "accidental overwrite" problem and is enough?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 throwing an error for this should be considered a requirement.
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.
In this option, this is an error, because one is attempting to overwrite
$arg1
with a new declaration. Presumably if we chose this option, it would be to ensure that this was an error.I happen to agree that there is a use case that needs to be accounted for here (I have made the argument elsewhere many times about declarations not knowing what all the values are externally)