search

cucumber / cucumber-expressions

Human friendly alternative to Regular Expressions
MIT License
148 stars 51 forks source link

`end` as a method name / prop can cause some issues #236

Open luke-hill opened 11 months ago

luke-hill commented 11 months ago

🤔 What's the problem you've observed?

In many of the AST sub-classes, such as Node there are a variety of attributes passed in. One of these is called end or derivations thereof. This name "end" can be problematic in some languages (Such as ruby). A few examples are below Java - https://github.com/cucumber/cucumber-expressions/blob/main/java/src/main/java/io/cucumber/cucumberexpressions/Ast.java#L64 Ruby - https://github.com/cucumber/cucumber-expressions/blob/main/ruby/lib/cucumber/cucumber_expressions/ast.rb#L38 JS - https://github.com/cucumber/cucumber-expressions/blob/main/javascript/src/Ast.ts#L77

✨ Do you have a proposal for making it better?

It seems as though we often use the hash representation methods, and not directly using the getters. Therefore I propose removing all getters and just rely on using the hash methods. Failing this maybe altering start/end to starting and ending

📚 Any additional context?

This text was originally generated from a template, then edited by hand. You can modify the template here.

luke-hill commented 11 months ago

ping @aslakhellesoy if you're around. @mpkorstanje also as a breakout to enable the refactor PR for ruby to be done.

I'll raise this as a discussion point next thurs for others to read/chat on. @davidjgoss / @ehuelsmann spring to mind as obvious ones as well as @WannesFransen1994

mpkorstanje commented 11 months ago

Therefore I propose removing all getters and just rely on using the hash methods.

For Ruby only? No problem. I don't know enough about Ruby to make a judgement there.

mpkorstanje commented 11 months ago

For the other languages I don't think this is necessarily as good idea.

In general we want to keep the code structure (folders, files, clas names, method names, ect) similar so we can compare the implementations.

And that means we can't use all the best practices for each language. But for Ruby, and this specific change, I don't see a problem because we're not changing the structure.

luke-hill commented 11 months ago

Do you think then maybe just renaming all the getter methods to something diff for that one method end is perhaps the best course of action (Maybe with a comment explaining why?)