-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor informal taxon groups usage according to api changes #701
Refactor informal taxon groups usage according to api changes #701
Conversation
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.
Lint job seems to have some issue.. I'll try rerun
* @param obj object to remove keys from | ||
* @param keys array of keys that should be removed | ||
*/ | ||
public static withNonNullableKeys<T extends {[prop: string]: any}, K extends keyof T>(obj: T): WithNonNullableKeys<T, K> { |
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.
Shouldn't it be removeNullableFields
or at least withNoNullableKeys
? This name sounds to me like you are adding extra keys rather than removing nullable keys
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.
another alternative: getNonNullableKeys
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.
The term NonNullable
comes from TypeScript: NonNullable
Though the "keys" word usage is weird... How about withNonNullableValues
? getNonNullableKeys
sounds to me like it would return keys of an object.
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.
I pushed withNonNullableValues
as a proposal. Updated the JS doc comment also. It was copy pasted from some other utility fn so there was an extra param.
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.
I still don't think I would immediately understand what withNonNullableValues
does without reading the comment. Usually the word with
I would associate with adding something to the result, eg. withLatestFrom
in rxjs. Maybe excludeNullables
or invert it selectNonNullables
.
Sorry this is getting rather pedantic... I'll give the PR an approve.
informal-taxon-groups/:id/parent
instead ofinformal-taxon-groups/:id/parents
(reasoning inImprove
/informal-taxon-groups/
laji-api#51)informal-list-breadcrumb
(currently broken in https://beta.laji.fi/taxon/browse?informalGroupFilters=MVL.342 - open the console and there are errors spitted)Closes #700