Split simplifyClaim and simplifySnak #122

maxlath · 2023-06-15T18:12:42Z

simplifyClaim and simplifySnak are currently aliases of the same function, which creates complexity and confusion, as a claim has itself a mainsnak. By splitting those functions, typing also becomes easier.

Any opinion @EdJoPaTo @mshd?

EdJoPaTo

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/simplify_claims.ts

EdJoPaTo · 2023-06-16T06:51:40Z

src/helpers/parse_snak.ts

 } as const

-export function parseClaim (datatype, datavalue, options, claimId) {
+export function parseSnak (datatype, datavalue, options) {


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?

Suggested change

export function parseSnak (datatype, datavalue, options) {

export function parseSnak (datatype: DataType, datavalue: SnakValue, options: any = {}) {

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 ^^

Actually, this seems to work a01f5ab, but hacky ><

Further addressed in 0805fee

src/helpers/simplify_claims.ts

EdJoPaTo · 2023-06-16T07:44:29Z

src/helpers/simplify_claims.ts

+  let valueObj
+  if (isPlainObject(value)) {
+    valueObj = value as CustomSimplifiedClaim
+  } else {
+    valueObj = { value } as CustomSimplifiedClaim
+  }


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)

Suggested change

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 }

}

addressed in 9755da5

EdJoPaTo · 2023-06-16T07:49:45Z

src/helpers/parse_snak.ts

+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


When explicitly specifying datatype: DataType this is an expected TypeScript error but we know it works so just ignore it.

Suggested change

// Known case requiring this: legacy "muscial notation" datatype

// @ts-expect-error Known case requiring this: legacy "musical notation" datatype

I can't find how to add @ts-expect-error, it tells me that it's an unused directive

src/helpers/simplify_claims.ts

src/types/simplify_claims.ts

src/types/claim.ts

src/helpers/parse_snak.ts

following suggestion #122 (comment)

following suggestions in #122 (comment) #122 (comment)

EdJoPaTo

I still like how the simplification reduces the complexity and therefore also the typing situation!

src/helpers/parse_snak.ts

following suggestion #122 (comment)

following suggestions in #122 (comment) #122 (comment)

src/helpers/parse_snak.ts

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)

EdJoPaTo suggested changes Jun 16, 2023

View reviewed changes

EdJoPaTo reviewed Jun 16, 2023

View reviewed changes

src/helpers/simplify_claims.ts Outdated Show resolved Hide resolved

maxlath force-pushed the split-simplify-claims-and-simplify-snaks branch from f3f8f24 to 0805fee Compare July 31, 2023 14:03

EdJoPaTo reviewed Jul 31, 2023

View reviewed changes

src/types/simplify_claims.ts Outdated Show resolved Hide resolved

src/types/claim.ts Outdated Show resolved Hide resolved

src/helpers/parse_snak.ts Outdated Show resolved Hide resolved

maxlath added a commit that referenced this pull request Aug 1, 2023

simplify_claims types: refactor SimplifiedReferenceSnaks

898722e

following suggestion #122 (comment)

maxlath force-pushed the split-simplify-claims-and-simplify-snaks branch from f8c5ed1 to 898722e Compare August 1, 2023 12:00

maxlath added a commit that referenced this pull request Aug 1, 2023

parseSnak: replace InconsistentDataType with @ts-expect-error

0056c1b

following suggestions in #122 (comment) #122 (comment)

EdJoPaTo reviewed Aug 1, 2023

View reviewed changes

src/helpers/parse_snak.ts Outdated Show resolved Hide resolved

EdJoPaTo suggested changes Aug 2, 2023

View reviewed changes

src/helpers/parse_snak.ts Outdated Show resolved Hide resolved

maxlath added a commit that referenced this pull request Dec 17, 2023

simplify_claims types: refactor SimplifiedReferenceSnaks

e9327be

following suggestion #122 (comment)

maxlath added a commit that referenced this pull request Dec 17, 2023

parseSnak: replace InconsistentDataType with @ts-expect-error

681c74c

following suggestions in #122 (comment) #122 (comment)

maxlath force-pushed the split-simplify-claims-and-simplify-snaks branch from bbc2d82 to d77cbd8 Compare December 17, 2023 11:07

EdJoPaTo reviewed Dec 17, 2023

View reviewed changes

src/helpers/parse_snak.ts Outdated Show resolved Hide resolved

maxlath and others added 13 commits March 3, 2024 22:01

split simplifyClaim and simplifySnak

3df975a

Now simplifySnak is applied to the claim mainsnak, but other claim-specific operations are performed by simplifyClaim alone

docs/simplify_claims.md: update

8eca25d

set keepOptions as const

1cd9320

Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>

alias simplifyReference as simplifyReferenceRecord with a deprecation…

531a483

… warning to prevent a breaking change, even if simplifyReferenceRecord was undocumented

add types to apply*Simplification functions

a6c74e6

add types to parseSnak function

82b59be

simplifyClaim: fix typing

d9a7f0d

parse_snak: include known inconsistent datatypes in DataType

03f7851

simplify_claims: add type to applyObjectSimplification simplified object

6cf1e53

simplify_claims types: refactor SimplifiedReferenceSnaks

e7a3f9f

following suggestion #122 (comment)

parseSnak: replace InconsistentDataType with @ts-expect-error

b1f388e

following suggestions in #122 (comment) #122 (comment)

parseSnak: refactor error handling

2a2437c

Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>

parseSnak: replace void type with undefined

740a737

as recommended in #122 (comment)

maxlath force-pushed the split-simplify-claims-and-simplify-snaks branch from d77cbd8 to 740a737 Compare March 3, 2024 21:01

maxlath merged commit 2aeed2c into main Mar 3, 2024

	export function parseSnak (datatype, datavalue, options) {
	export function parseSnak (datatype: DataType, datavalue: SnakValue, options: any = {}) {

	// Known case requiring this: legacy "muscial notation" datatype
	// @ts-expect-error Known case requiring this: legacy "musical notation" datatype

Uh oh!

Split simplifyClaim and simplifySnak #122

Split simplifyClaim and simplifySnak #122

Uh oh!

Conversation

maxlath commented Jun 15, 2023

Uh oh!

EdJoPaTo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EdJoPaTo Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

maxlath Jun 18, 2023

Choose a reason for hiding this comment

Uh oh!

maxlath Jun 18, 2023

Choose a reason for hiding this comment

Uh oh!

maxlath Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EdJoPaTo Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxlath Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

EdJoPaTo Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxlath Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EdJoPaTo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EdJoPaTo Jun 16, 2023 •

edited

Loading

EdJoPaTo Jun 16, 2023 •

edited

Loading