Skip to content
This repository was archived by the owner on Mar 25, 2021. It is now read-only.

Add UnitValue type to QueryRepresentation#219

Merged
micgro42 merged 2 commits into
masterfrom
queryRepUnitValues
Mar 12, 2021
Merged

Add UnitValue type to QueryRepresentation#219
micgro42 merged 2 commits into
masterfrom
queryRepUnitValues

Conversation

@guergana
Copy link
Copy Markdown
Contributor

…unt for unit values

@guergana guergana force-pushed the queryRepUnitValues branch 2 times, most recently from f1add93 to 37c45f1 Compare March 11, 2021 13:33
@guergana guergana changed the base branch from master to QuantityValueInterface March 11, 2021 13:34
@guergana guergana force-pushed the queryRepUnitValues branch 2 times, most recently from 3512923 to 2756f81 Compare March 11, 2021 14:15
Comment thread src/sparql/TripleBuilder.ts Outdated
return value;
}
if ( 'value' in value ) { // if value is UnitValue
return value.value; // TODO: multiply by unit, define how to manage it
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are we doing any unit conversion at some point??

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, as it was discussed and documented, we will have two different queries depending on whether this is a unit or not.

In this specific case here I would suggest to throw an error after the first if, instead of returning a number. That is because that number will still lead to a wrong query and so it is better to fail explicitly and loudly.

The proper implementation will be done in the course of T276938.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Comment thread src/sparql/TripleBuilder.ts Outdated
private buildTripleForExplicitValue( datatype: string, value: string | number ): Term {
switch ( datatype ) {
case 'string':
case 'quantity':
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line is probably causing the browser test failure. We still want to treat quantity as a datatype with only limited support in this pull request. We'll build the SPARQL in https://phabricator.wikimedia.org/T276938

@guergana guergana force-pushed the QuantityValueInterface branch from a2ca220 to bf2527c Compare March 12, 2021 08:32
@guergana guergana force-pushed the queryRepUnitValues branch 2 times, most recently from 1a932cb to 54a608c Compare March 12, 2021 09:30
Base automatically changed from QuantityValueInterface to master March 12, 2021 09:30
@guergana guergana force-pushed the queryRepUnitValues branch 2 times, most recently from 35af87f to bc048dd Compare March 12, 2021 10:23
@guergana guergana force-pushed the queryRepUnitValues branch from bc048dd to 94a5ed6 Compare March 12, 2021 10:29
expect( actual ).toStrictEqual( expected );
} );

it( 'with quantity value', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added this test to pass sonarcloud!!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me

Comment thread src/sparql/TripleBuilder.ts Outdated
expect( actual ).toStrictEqual( expected );
} );

it( 'with quantity value', () => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me

Co-authored-by: Michael Große <michael.grosse@wikimedia.de>
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

91.7% 91.7% Coverage
0.0% 0.0% Duplication

@guergana guergana requested a review from micgro42 March 12, 2021 14:13
@micgro42 micgro42 changed the title Adjust value type in QueryRepresentation/Condition interfaces to acco… Add UnitValue type to QueryRepresentation Mar 12, 2021
@micgro42 micgro42 merged commit a65e894 into master Mar 12, 2021
@micgro42 micgro42 deleted the queryRepUnitValues branch March 12, 2021 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants