Use name_plural and display_name_plural#861
Conversation
eliasjpr
left a comment
eliasjpr
left a comment
There was a problem hiding this comment.
Why not just create a variable called name_plural since its the only variable that gets pluralized? Then everywhere where pluralize(@name) gets replaced with name_plural
@eliasjpr Nice idea 👍 |
|
@eliasjpr Done! 🎉 |
|
|
||
| response.status_code.should eq(200) | ||
| response.body.should contain("<%= Inflector.pluralize(display_name) %>") | ||
| response.body.should contain("<%= pluralize(display_name) %>") |
There was a problem hiding this comment.
Thank you, Done! 👍
| exit 1 | ||
| end | ||
|
|
||
| @name_plural = Inflector.pluralize(word) |
There was a problem hiding this comment.
Where does word come from? Isn't this pluralizing @name?
Also, any reason not to create a getter, or write a custom one like this:
def plural_name
@plural_name ||= Inflector.pluralize(@name)
endThere was a problem hiding this comment.
Hi @marksiemers 🎉
You're completely right, let me fix that 💯
There was a problem hiding this comment.
Also, any reason not to create a getter, or write a custom one like this:
Is a getter really needed? I think initialize method already does the job, no?
@name_plural is initialized just one time inside the constructor, so no memoization requirement
There was a problem hiding this comment.
It can be done without the getter, assuming it will never be needed publicly.
If it will ever be needed publicly, then creating a private getter now will make things more consistent when that change comes.
I know this isn't ruby, but one of the ruby idioms that I follow is that you minimize directly accessing instance variables (ideally only in the constructor/initialize method).
There was a problem hiding this comment.
@marksiemers Yeah, nice suggestion 👍
I think we can refactor this in a new PR (@name_plural and others) 😅
There was a problem hiding this comment.
BTW, nice to see you here again 😄 👍
|
@faustinoaq - Thanks. I started a new job, moved, and got into rock climbing - so my OSS time has been very limited. It looks like the CI is currently failing due to Crystal 0.25.0 incompatibilities - I'm assuming that is a bit of a priority - should this PR just wait for those fixes? Also for this change, if you switch to a getter method, if anything changes in the implementation of that method, you won't have to change 25 files again. Of course, if the name of the method changes, then we're still stuck with changing all the files. |
Hehe, nice, no problem 👍
Yeah, almost all current PRs depend on #857 😅
Yeah, you're right, but I think would be nice to change all other ivars as well in a new PR. This would be a bigger refactor. We're using a lot of ivars in all templates 😅 |
|
@marksiemers I guess I was a bit confused about this change (too many files 😅 ) Your're right, is better to use a |
faustinoaq
changed the title
@marksiemers Done 👍 |
4 participants
Description of the Change
This PR simplifies pluralization by using
pluralizemethod@name_pluralvariable instead ofInflector.pluralizeAlternate Designs
Keep
Inflector.pluralizeBenefits
Less verbose and fixes #713
Possible Drawbacks
No