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

MBS-486: Add support for years BCE #1723

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented Oct 1, 2020

Implement MBS-486

We have had kinda semi-broken BCE support for decades, but we have legitimate artists who were born and died BCE, and places old enough that they were founded BCE, for example. This adds proper support for BCE dates.

The dates are stored in standard astronomical format (1 BCE = 0) but they are always shown to the user as the more intuitive version without a year 0 on the website (including for entering and editing). On the API, which is mostly meant for machine usage, the astronomical format is retained.

Year 0 is blocked when editing since it's not a valid year in BCE style. I added a more precise error when validating and in React forms. I did not bother to implement the same error in TT, since the date is rejected as invalid there anyway, which is still correct (just slightly less clear).

@reosarevok reosarevok added the New feature Non urgent new stuff label Oct 1, 2020
@reosarevok reosarevok force-pushed the MBS-486 branch 4 times, most recently from d1eee2a to 76de4e1 Compare October 1, 2020 19:52
@reosarevok reosarevok marked this pull request as ready for review October 1, 2020 20:26
@reosarevok reosarevok requested review from yvanzo and mwiencek and removed request for yvanzo October 1, 2020 20:26
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

I didn't test this yet, but just a few small things I noticed so far

lib/MusicBrainz/Server/Form/Field/DatePeriod.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Form/Field/PartialDate.pm Outdated Show resolved Hide resolved
root/static/scripts/common/utility/formatDate.js Outdated Show resolved Hide resolved
root/static/scripts/edit/utility/dates.js Outdated Show resolved Hide resolved
root/static/scripts/edit/utility/dates.js Outdated Show resolved Hide resolved
root/utility/age.js Outdated Show resolved Hide resolved
@reosarevok reosarevok force-pushed the MBS-486 branch 3 times, most recently from 2ea5ae5 to 3c10d0a Compare October 2, 2020 06:56
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

This seems like a good start.

I can't enter "-0001" in the release or relationship editors, but I'm not sure these are useful there (i.e. if it's worth fixing the validation in two more places). Anyway, those cases will likely start working as we consolidate the date period components in the future.

@reosarevok
Copy link
Member Author

It might be useful in the relationship editor (but quite rarely) and should probably never happen in the release editor anyway. I could add it to the relationship editor, if we want to. We might need to check if it anything needs to be changed in the alias form fields if those get merged sooner.

@reosarevok
Copy link
Member Author

reosarevok commented Sep 16, 2021

Rebased this on current master, seems to actually work fine in the alias form too, so hopefully it's still fine!

Bringing this back because the few ones we already have are causing issues, see https://community.metabrainz.org/t/bc-dates-issue/551104 :)

@yvanzo
Copy link
Contributor

yvanzo commented Sep 23, 2021

Wouldn’t that break JSON+LD given https://schema.org/Date?

@reosarevok
Copy link
Member Author

Well, in theory ISO_8601 supports BC years (https://en.wikipedia.org/wiki/ISO_8601 says so at least) and I assume given there's plenty of philosophers and whatnot probably supported by schema.org, they are able to parse those. But I haven't actively tested JSON-LD on these, are you seeing issues, other than the year being negative?

@yvanzo
Copy link
Contributor

yvanzo commented Sep 27, 2021

Well, in theory ISO_8601 supports BC years (https://en.wikipedia.org/wiki/ISO_8601 says so at least)

Not like that; See Monkey’s comment about it. It requires at least five digits or it can be mistaken with a shortened notation.

(By the way, that comment has been entered during Summit 19; See related notes.)

@reosarevok
Copy link
Member Author

Hmm. I'm not sure I'm seeing the issue. My understanding is that if it goes over the 4 digits (plus - sign) then we need to have a specified number of extra digits, but the example just after for example mentions that 2BC = −0001. The LOC specs for example have 4 digits by default, it seems: http://www.loc.gov/standards/datetime/

@reosarevok reosarevok removed this from the 2021-10-04 milestone Sep 28, 2021
@yvanzo yvanzo added this to the 2021-10-18 milestone Sep 29, 2021
@reosarevok reosarevok force-pushed the MBS-486 branch 2 times, most recently from 75a3cf6 to d5d516f Compare October 6, 2021 14:05
@reosarevok reosarevok modified the milestones: 2021-10-25, 2021-11-08 Oct 20, 2021
@reosarevok reosarevok removed this from the 2021-11-15 milestone Nov 15, 2021
@yvanzo yvanzo added this to the 2021-11-29 milestone Nov 17, 2021
@reosarevok reosarevok modified the milestones: 2021-11-29, 2021-12-12 Nov 23, 2021
@reosarevok reosarevok modified the milestones: 2021-12-13, 2021-12-27 Dec 8, 2021
@reosarevok reosarevok modified the milestones: 2021-01-31, 2022-02-14 Feb 1, 2022
@reosarevok reosarevok removed this from the 2022-02-14 milestone Feb 9, 2022
@reosarevok reosarevok force-pushed the MBS-486 branch 2 times, most recently from 5b35d32 to bfab2b4 Compare January 12, 2023 16:57
@reosarevok reosarevok changed the title MBS-486: Add support for years BC MBS-486: Add support for years BCE Jan 12, 2023
@reosarevok
Copy link
Member Author

reosarevok commented Jan 12, 2023

This now works on relationships, since they use the React components. It doesn't work on the release editor, but given BCE releases are AFAICT not a thing, it's probably fine.

That said, there's currently a bug with relationships - we're meant to enter the year as BCE, but store it as astronomical. As such, since formatDate adapts the date by removing 1 from the year if it's 0 or less, the relationship editor shows not-yet entered dates as one year earlier than it should.

@mwiencek
Copy link
Member

mwiencek commented Mar 3, 2023

If I understand the issue correctly, PartialDateInput stores dates in BCE format in its state. And I guess we can't change that because other pages using it (edit profile form, alias edit form, external link attribute dialog) have to post BCE dates to the server.

Perhaps we can change formatDate to accept an additional argument that specifies what format the input date is in?

@reosarevok
Copy link
Member Author

Yeah. I was just not sure how to tell whether the date is new (state-only) or old - how would you keep track in a sane way?

We have had kinda semi-broken BCE support for decades, but we
have legitimate artists who were born and died BCE, and places
old enough that they were founded BCE, for example. This adds
proper support for BCE dates.

The dates are stored in standard astronomical format (1 BCE = 0)
but they are always shown to the user as the more intuitive version
without a year 0 on the website (including for entering and editing).
On the API, which is mostly meant for machine usage,
the astronomical format is retained.

Year 0 is blocked when editing since it's not a valid year in BCE
style. I added a more precise error when validating and in React
forms. I did not bother to implement the same error in TT, since
the date is rejected as invalid there anyway, which is still correct
(just slightly less clear).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Non urgent new stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants