-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
src/client/entity-editor/validators/
- Copy a few utilities from bookbrainz-site/src/client/helpers/utils.tsx - Replace `IdentifierType` duck types with existing complete definition
test/src/client/entity-editor/validators/
This way we can return the reason for the failed validation instead of just a boolean.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Problem
We have two nearly duplicate sets of entity validation functions in
bookbrainz-site
andbookbrainz-utils
which should ideally be part ofbookbrainz-data
and then can be used by both.Solution
bookbrainz-site
into this repository and make sure the tests still pass (yarn build-js-for-test && mocha test/validators
).ValidationError
with the reason instead of just a boolean.bookbrainz-utils
.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
bookbrainz-site
where a complexity of 50 is allowed.language
which is the ID in forms and for the validators while it is the lazy-loaded object forlanguageId
elsewhere. We could change these inbookbrainz-site
to make the code and the types cleaner.bookbrainz-utils
(Use unified entity validators bookbrainz-utils#46)bookbrainz-site
(should be even more straightforward than forbookbrainz-utils
)bookbrainz-utils
bookbrainz-site
bookbrainz-site
which had to be copied over intobookbrainz-data
or are otherwise unused (I have a list which I will share)