Remove several methods - Githubissues

wmde / DataValuesJavaScript

JavaScript implementations of all basic DataValue classes, associated parsers and formatters

http://wikiba.se

Other

5 stars 1 forks source link

Remove several methods #134

Closed Ladsgroup closed 4 years ago

Ladsgroup commented 4 years ago

GlobeCoordinate. getDecimal has not been used since a50a48e5cedcb5
QuantityValue.get(UpperBound|LowerBound|Amount) has not been used since fadb3ab8b5c

lucaswerkmeister commented 4 years ago

Okay, I have to admit, removing the QuantityValue methods feels strange to me. What are we even using this class for if we don’t use those getters at all? (And I suspect the answer is, we only construct instances of the class and then serialize them to JSON, without processing them very much.)

Anyways, couldn’t getUnit be removed then as well? As far as I can tell it’s not used either.

Ladsgroup commented 4 years ago

Anyways, couldn’t getUnit be removed then as well? As far as I can tell it’s not used either.

It's used in QuantityInput.js

lucaswerkmeister commented 4 years ago

Anyways, couldn’t getUnit be removed then as well? As far as I can tell it’s not used either.

It's used in QuantityInput.js

Which file is that? I can’t find it in this repository or in Wikibase.

Ladsgroup commented 4 years ago

Anyways, couldn’t getUnit be removed then as well? As far as I can tell it’s not used either.

It's used in QuantityInput.js

Which file is that? I can’t find it in this repository or in Wikibase.

https://gerrit.wikimedia.org/r/plugins/gitiles/data-values/value-view/+/refs/heads/master/src/experts/QuantityInput.js

lucaswerkmeister commented 4 years ago

Okay, thanks.

I’m still not sure about those QuantityValue removals, though. This feels different than a lot of the other unused code removals – this isn’t much code, at least not after minification, and the class feels incomplete without those getters (especially if getUnit() is left behind because that one happens to be used at the moment – but if we want to use any of the other getters in the future, we’ll first have to publish a new release of this library to add them back, or use the internal members directly).

(No objections from me to removing GlobeCoordinate.getDecimal, by the way.)

Ladsgroup commented 4 years ago

I’m still not sure about those QuantityValue removals, though. This feels different than a lot of the other unused code removals – this isn’t much code, at least not after minification, and the class feels incomplete without those getters (especially if getUnit() is left behind because that one happens to be used at the moment – but if we want to use any of the other getters in the future, we’ll first have to publish a new release of this library to add them back, or use the internal members directly).

I agree with you and would have not done it if it was a couple of years ago but now the long term plan is to burn all of this code in fire and replace everything with a more modern code. Regarding the release: These codes are submodules currently, I just bump it to HEAD (no need to release anything).

lucaswerkmeister commented 4 years ago

Hm, okay.

lucaswerkmeister commented 4 years ago

Though that makes me wonder if we should add a “0.11.0 (dev)” to the README with this, even if we don’t bump the version in package.json yet?

Ladsgroup commented 4 years ago

Though that makes me wonder if we should add a “0.11.0 (dev)” to the README with this, even if we don’t bump the version in package.json yet?

I was like "I'm sure I wrote this" and then I looked and realized I haven't "git add"ed it.