Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Status: Active » Needs review
StatusFileSize
new1.23 KB

Simple patch attached, I think there is no real discussion needed about the labels, and it seems to work.

One question is whether we now want to also use entities' plural labels somewhere in the module? Could justify a look. (Or we can just keep our eyes open to see if we spot something.)

rosinegrean’s picture

I saw some comments from @bojanz on related tasks about the last commas, here is an example: https://www.drupal.org/node/2702745#comment-11052643

but @amateescu who was running the sprint where these changes were made, did not really agree they were mandatory.
However, for https://www.drupal.org/node/2703323 i've tested with the commas and had no issues.

As far as i know, there is no standard on this. Or am i wrong ?

  • drunken monkey committed 1a164e1 on 8.x-1.x
    Issue #2701929 by drunken monkey, prics: Added plural labels to our...
drunken monkey’s picture

Status: Needs review » Fixed

As written in the other issue, I actually thought that including those trailing commas gave you a parse error (like in JSON). But I now see we already have those in some places, so if this ever was the case, it apparently changed quite some time ago.
All the better, I prefer it with trailing commas, too. Thanks for pointing this out!

Added the commas and committed.
Thanks again!

vaibhavjain’s picture

Hey Drunken Monkey, Prics,

I am using 8.0.x and the above said patch may work only for 8.1.x as PluralTranslation is not available in 8.0.x
I can go ahead and undo changes in this patch, just wanted to know if this will have any impact on the working of this module.
Also, if that is the case, please have a release for 8.0.x or mark the module for 8.1.x only.

Thanks.

drunken monkey’s picture

Drupal 8.0.x has reached its end of life as of ten days ago, if I understand correctly, so I don't think we have to provide any compatibility with that – especially since our own module isn't stable yet.
I might have misunderstood the new release cycle, but unless I'm mistaken, you should probably get used to updating pretty soon after a new minor version of Core is released. And after a Core version's last security release window, I wouldn't count on any new versions of contrib modules to be compatible with that anymore.

But to answer your question, you can just remove the patch, it doesn't have any real consequences.

vaibhavjain’s picture

Hey,

I wasn't aware of this release cycle.
I have removed the code, and things looks to be good until now.

Thanks for the reply.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.