Conversation
8908ead to
2cc6546
Compare
language/reserved_keywords.rb
Outdated
| # Copied from Prism::Translation::Ripper | ||
| keywords = [ |
There was a problem hiding this comment.
TODO: It would be nice to have one canonical list that can be used in all these places:
spec/ruby/language/hash_spec.rbspec/ruby/language/reserved_keywords.rblib/prism/translation/ripper.rb- Ideally, it would also be nice to generate
doc/keywords.rdocfrom that list, to ensure that all keywords are documented.
Where should such a canonical list live?
There was a problem hiding this comment.
There is no way to avoid duplication there, since it's files in different repositories.
So for ruby/spec let's have it as a new file under language/fixtures.
I think a %w[] array would be nice for this BTW.
FWIW in ruby/ruby there is also defs/keywords.
eregon
left a comment
There was a problem hiding this comment.
Adding these specs seems good but they should match the existing style and structure of ruby/spec, I'll review in more details once it does.
language/reserved_keywords.rb
Outdated
| __LINE__ | ||
| ] | ||
|
|
||
| keywords.each do |kw| |
There was a problem hiding this comment.
| keywords.each do |kw| | |
| keywords.each do |name| |
kw feels too confusing here, especially since there is a spec about keyword arguments below.
Since we are mostly testing names for variables/methods, name seems clearer in usages.
There was a problem hiding this comment.
Mmmmm I disagree, in a spec focused on describing the behaviour of keywords, I would expect "keyword" to a be prominent term. It's an overloaded term, but that's just how Ruby is already. Perhaps keyword is better than kw?
Anyway, I renamed to name
|
Thanks for taking another stab at this. |
amomchilov
left a comment
There was a problem hiding this comment.
OK, I think that's everything addressed.
I kept the history in tract to make it easier to review, but I'll squash and rebase before merging
language/reserved_keywords.rb
Outdated
| __LINE__ | ||
| ] | ||
|
|
||
| keywords.each do |kw| |
There was a problem hiding this comment.
Mmmmm I disagree, in a spec focused on describing the behaviour of keywords, I would expect "keyword" to a be prominent term. It's an overloaded term, but that's just how Ruby is already. Perhaps keyword is better than kw?
Anyway, I renamed to name
eregon
left a comment
There was a problem hiding this comment.
I missed your recent replies, but looks all good, thanks!
Add specifications for all the reserved keywords, documenting which of them can be used as: