-
Notifications
You must be signed in to change notification settings - Fork 48
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
Split simplifyClaim and simplifySnak #122
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.
I really like the approach to reduce complexity and confusion which also simplifies the typings!
I have a bunch of suggestions. For now I haven't continued looking more into it so there might be more things when the current batch of suggestion is handled.
Thank you for asking for feedback on it!
src/helpers/parse_snak.ts
Outdated
@@ -98,7 +98,7 @@ export const parsers = { | |||
'wikibase-sense': entity, | |||
} as const | |||
|
|||
export function parseClaim (datatype, datavalue, options, claimId) { | |||
export function parseSnak (datatype, datavalue, options) { |
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.
Can we provide typings here in an easy way? There are DataType
(which needs refactoring in the future but still) and SnakValue
.
Not sure what the correct options type would be? But better explicitly any
than nothing. Also the = {}
is probably missing currently?
export function parseSnak (datatype, datavalue, options) { | |
export function parseSnak (datatype: DataType, datavalue: SnakValue, options: any = {}) { |
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.
This would need more work due to the hacks on datatype this function does:
// Known case of missing datatype: form.claims, sense.claims
datatype = datatype || datavalue.type
// Known case requiring this: legacy "muscial notation" datatype
datatype = datatype.replace(' ', '-')
I would let that for another PR ^^
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.
Actually, this seems to work a01f5ab, but hacky ><
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.
Further addressed in 0805fee
src/helpers/simplify_claims.ts
Outdated
let valueObj | ||
if (isPlainObject(value)) { | ||
valueObj = value as CustomSimplifiedClaim | ||
} else { | ||
valueObj = { value } as CustomSimplifiedClaim | ||
} |
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.
It should be like the following suggestion but that's currently not possible as it results in a bunch of type errors…
- let valueObj
- if (isPlainObject(value)) {
- valueObj = value as CustomSimplifiedClaim
- } else {
- valueObj = { value } as CustomSimplifiedClaim
- }
+ let valueObj: CustomSimplifiedClaim
+ if (isPlainObject(value)) {
+ valueObj = value
+ } else {
+ valueObj = { value }
+ }
For now I would suggest to just type it explicitly as any
so it's noticed easier later on and can be fixed on its own rather than in this PR. (When DataType
is refactored it should solve itself then)
let valueObj | |
if (isPlainObject(value)) { | |
valueObj = value as CustomSimplifiedClaim | |
} else { | |
valueObj = { value } as CustomSimplifiedClaim | |
} | |
let valueObj: any // TODO: type as CustomSimplifiedClaim | |
if (isPlainObject(value)) { | |
valueObj = value | |
} else { | |
valueObj = { value } | |
} |
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.
addressed in 9755da5
src/helpers/parse_snak.ts
Outdated
@@ -98,7 +98,7 @@ export const parsers = { | |||
'wikibase-sense': entity, | |||
} as const | |||
|
|||
export function parseClaim (datatype, datavalue, options, claimId) { | |||
export function parseSnak (datatype, datavalue, options) { | |||
// Known case of missing datatype: form.claims, sense.claims | |||
datatype = datatype || datavalue.type | |||
// Known case requiring this: legacy "muscial notation" datatype |
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.
When explicitly specifying datatype: DataType
this is an expected TypeScript error but we know it works so just ignore it.
// Known case requiring this: legacy "muscial notation" datatype | |
// @ts-expect-error Known case requiring this: legacy "musical notation" datatype |
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 can't find how to add @ts-expect-error
, it tells me that it's an unused directive
f3f8f24
to
0805fee
Compare
following suggestion #122 (comment)
f8c5ed1
to
898722e
Compare
following suggestions in #122 (comment) #122 (comment)
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 like how the simplification reduces the complexity and therefore also the typing situation!
following suggestion #122 (comment)
following suggestions in #122 (comment) #122 (comment)
bbc2d82
to
d77cbd8
Compare
Now simplifySnak is applied to the claim mainsnak, but other claim-specific operations are performed by simplifyClaim alone
Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
… warning to prevent a breaking change, even if simplifyReferenceRecord was undocumented
following suggestion #122 (comment)
following suggestions in #122 (comment) #122 (comment)
Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
as recommended in #122 (comment)
d77cbd8
to
740a737
Compare
simplifyClaim
andsimplifySnak
are currently aliases of the same function, which creates complexity and confusion, as a claim has itself amainsnak
. By splitting those functions, typing also becomes easier.Any opinion @EdJoPaTo @mshd?