Skip to content
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

front: rename variable from French to English #7893

Merged

Conversation

turnerian2004
Copy link
Contributor

Resolves #7891

@turnerian2004 turnerian2004 requested a review from a team as a code owner June 30, 2024 05:05
@emersion emersion requested a review from clarani July 1, 2024 05:47
@emersion
Copy link
Member

emersion commented Jul 1, 2024

Thanks for submitting a pull request!

The idea looks good, but I wonder if searchRollingStock, searchStock or just search would be a better pick? We use the term "rolling stock" as the translation of the French word "matériel roulant". It should be clear enough from the function name useFilterRollingStock() that this is about rolling stocks (and most other variable names don't have "rolling stock" in their name).

What do you think?

@turnerian2004 turnerian2004 changed the title front: rename French variable name to English name front: rename variable from French to English Jul 1, 2024
@turnerian2004
Copy link
Contributor Author

Thanks for submitting a pull request!

The idea looks good, but I wonder if searchRollingStock, searchStock or just search would be a better pick? We use the term "rolling stock" as the translation of the French word "matériel roulant". It should be clear enough from the function name useFilterRollingStock() that this is about rolling stocks (and most other variable names don't have "rolling stock" in their name).

What do you think?

Thank you, Emersion, for the warm welcome and the translation explanation.

Yes, I agree with you that searchMaterial is not the best variable name. My vote is for searchStock, since that seems to most adhere to the codebase naming convention & provides more clarity than search.

@emersion
Copy link
Member

emersion commented Jul 1, 2024

Works for me!

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi turnerian2004, thanks for contributing to osrd and spotting this translation issue !

My vote goes to searchRollingStock. I think searchStock is a little confusing and feels like we don't want to bother to put the whole "rolling stock" word in the variable ? Moreover, everywhere in the app, we use rollingStock to refer to the train traction "material".

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@emersion emersion added this pull request to the merge queue Jul 2, 2024
Merged via the queue into OpenRailAssociation:dev with commit af83be8 Jul 2, 2024
17 checks passed
@turnerian2004
Copy link
Contributor Author

Hi SharglutDev! Thank you, also, for the warm welcome.

I started to type a reply to your comment but fell asleep before the posting the comment and the pr was merged lol

@turnerian2004
Copy link
Contributor Author

Hi SharglutDev! Thank you, also, for the warm welcome.

I started to type a reply to your comment but fell asleep before the posting the comment and the pr was merged lol

@SharglutDev 😊😊😊

@SharglutDev
Copy link
Contributor

Haha don't worry, thank you again for contributing, don't work too much !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants