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

Add custom pluralisation for the new Product Safety Alerts, Reports and Recalls #2335

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

callumknights
Copy link
Contributor

Add custom pluralisation for the new Product Safety Alerts, Reports and Recalls
specialist finder.

Trello: https://trello.com/c/ojsFViiX/2746-create-new-specialist-document-and-finder-product-safety-alerts-unsafe-products-and-recalls-3-5

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

@govuk-ci govuk-ci temporarily deployed to government-f-product-sa-xg72xt January 13, 2022 16:36 Inactive
@@ -224,6 +224,9 @@ en:
publication:
one: Publication
other: Publications
product-safety-alert-report-recalls:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct for this key to be plural when the others (e.g. L224, publication) are not?

(I know nothing about pluralisation rules so just making a guess based on consistency.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out Ben, getting a bit lost in singular vs plural at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted 👍

@callumknights callumknights force-pushed the product-safety-alert-pluralisation branch from 7843195 to becc935 Compare January 13, 2022 17:12
@govuk-ci govuk-ci temporarily deployed to government-f-product-sa-xg72xt January 13, 2022 17:12 Inactive
@nicholsj
Copy link
Contributor

I think it would be good to comment (either here or ideally in the code) to say why this is necessary. Otherwise it could look like an unintentional error.

@callumknights
Copy link
Contributor Author

There is actually some prior art for this with the medical-safety-alert key, so I'm wondering if a comment in the code is necessary. Should anyone think that's a mistake in future it's easy enough to find this PR in Github. I'll say for anyone else reading this in future that the reason for this pluralisation change is that there should be no reason in the frontend so display the name of this finder in the singular, as it doesn't read correctly. There are a few instances where it's named in the singular in specialist-publisher in the code, or even half plurals where just "recalls" is plural, but that's to work around instances where model's are called using things like document_type.pluralise.new which without custom inflections in the locales (which specialist-publisher doesn't have) just adds a trailing "s".

@benjamineskola
Copy link
Contributor

I'm inclined to think it's safe, yeah, @callumknights — looking at it, I can see it would be awkward if used singularly ("product safety alert, report, or recall"?) and also that the automatic pluralisation would get it wrong (because it wouldn't pluralise "alert" or "report", only "recall").

@nicholsj
Copy link
Contributor

Ok, thanks both. Happy to go with your recommendations - in any case, this conversation adds some detail!

@benjamineskola benjamineskola merged commit fcd486b into main Jan 20, 2022
@benjamineskola benjamineskola deleted the product-safety-alert-pluralisation branch January 20, 2022 11:08
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.

4 participants