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

Unify entity validators #316

Merged
merged 12 commits into from
Jun 26, 2024
Merged

Conversation

kellnerd
Copy link
Contributor

@kellnerd kellnerd commented Jun 26, 2024

Problem

We have two nearly duplicate sets of entity validation functions in bookbrainz-site and bookbrainz-utils which should ideally be part of bookbrainz-data and then can be used by both.

Solution

  • Copy the validators and their tests from bookbrainz-site into this repository and make sure the tests still pass (yarn build-js-for-test && mocha test/validators).
  • Refactor validators to throw a ValidationError with the reason instead of just a boolean.
  • Adapt tests to expect errors. (Luckily this was mostly a search and replace job with the proper regular expressions.)
  • Copy type definitions for the validated entity sections from bookbrainz-utils.
  • Integrate specific differences of the validators from bookbrainz-utils. Apart from the thrown error messages, which can be used for logging, and a few outdated/wrong checks this would be a912ab9 only!

Changes are probably best reviewed commit by commit.

Areas of Impact

None so far, the unified validators will only be used in the other two repos.

Remaining Tasks

  • Mute the stupid "Function 'dateValidator' has a complexity of 24. Maximum allowed is 20 complexity" error? This was no problem in bookbrainz-site where a complexity of 50 is allowed.
  • Change our expectations for some validators, they don't make much sense (see my TODO code comments)
  • There are also some inconsistent property names like language which is the ID in forms and for the validators while it is the lazy-loaded object for languageId elsewhere. We could change these in bookbrainz-site to make the code and the types cleaner.
  • Use validators for the import consumer in bookbrainz-utils (Use unified entity validators bookbrainz-utils#46)
  • Use validators in bookbrainz-site (should be even more straightforward than for bookbrainz-utils)
  • Delete validators in bookbrainz-utils
  • Delete validators in bookbrainz-site
  • Delete utils in bookbrainz-site which had to be copied over into bookbrainz-data or are otherwise unused (I have a list which I will share)

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

That's a lot of validators!

Nice work keeping it all clean and not getting jumbled up in the sheer amount of it :)

I think we are ready to merge as-is, and I'll deploy a new version of the ORM

export type AuthorCreditRow = {
author: EntityStub;
joinPhrase?: string;
name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

In bb-site the type also defines position, authorBBID and authorCreditID props. However, these are not present when creating/editing an entity, only when viewing it.
I suppose considering the use of these validators we can ignore it for now, although I wonder if that will affect you in the near future when you start parsing and consuming edition entities.
Let's revisit it then.
https://github.com/metabrainz/bookbrainz-site/blob/2315cb7707a31dc096758c03d7df128f15588d1e/src/client/entity-editor/author-credit-editor/actions.ts#L44-L51

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I was unsure about the exact need for this type definition as well.
Adding it was mostly an afterthought as all other sections had a type definition and I wanted the basic type to be there in the ORM without having to do another release in case I need this type soon.

@MonkeyDo MonkeyDo merged commit 7d1df5a into metabrainz:master Jun 26, 2024
2 checks passed
@kellnerd kellnerd deleted the entity-validators branch July 15, 2024 16:15
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.

2 participants