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

Fix grammar for message when only one commit is abandoned after Git import #3192

Closed
wants to merge 1 commit into from

Conversation

emesterhazy
Copy link
Collaborator

Before:

  • "Abandoned 1 commits that are no longer reachable."

After:

  • "Abandoned 1 commit that is no longer reachable."

…mport

Before:

- "Abandoned 1 commits that are no longer reachable."

After:

- "Abandoned 1 commit that is no longer reachable."
@yuja
Copy link
Collaborator

yuja commented Mar 3, 2024

Before:
* "Abandoned 1 commits that are no longer reachable."
After:
* "Abandoned 1 commit that is no longer reachable."

I'm not strongly against this, but I personally prefer not having singular/plural conditionals everywhere. I don't think it's worth the added complexity.

@joyously
Copy link

joyously commented Mar 3, 2024

I think it's best handled within a translation framework.

@emesterhazy
Copy link
Collaborator Author

RE: complexity, I agree. It's not ideal. But the current message reads very oddly when only one commit is abandoned. This is definitely just polish, so we could ignore it for now and implement something a little less ad-hoc.

RE: A translation framework. I'm not very familiar with how that might work, but I guess that's probably true. Maybe https://github.com/unicode-org/icu4x if the goal is to actually output messages in different languages.

@zummenix
Copy link
Collaborator

zummenix commented Mar 3, 2024

This message actually also bothered me initially. One alternative I can think of: Abandoned 1 commit(s) that are no longer reachable.

@martinvonz
Copy link
Owner

FWIW, I agree with Yuya. I don't even think of "1 commits" as sounding weird (and my native language does have plurals, in case you're wondering). But it does seem to bother many others because we've seen several patches like this. I think there are many more places to update if you want to do it consistently :)

@emesterhazy
Copy link
Collaborator Author

Thanks everyone for the feedback. I think that we should add this polish, but it should probably be done in a less ad-hoc way per the suggestion from @joyously. I'll drop this PR for now and come back with something better in the future.

@emesterhazy emesterhazy closed this Mar 4, 2024
@emesterhazy emesterhazy deleted the push-znwynrlsqwvl branch March 4, 2024 16:01
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

5 participants