Skip to content
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

About signature for toUnit() function #837

Closed
IndrajeetPatil opened this issue Mar 23, 2022 · 8 comments · Fixed by #839
Closed

About signature for toUnit() function #837

IndrajeetPatil opened this issue Mar 23, 2022 · 8 comments · Fixed by #839

Comments

@IndrajeetPatil
Copy link
Member

Is it just me or the signature for toUnit() function

toUnit(quantityOrDimension, values, targetUnit, molWeight, sourceUnit, molWeightUnit)

should instead be?

toUnit(quantityOrDimension, values, targetUnit, sourceUnit, molWeight, molWeightUnit)

As a new user, I was constantly befuddled by the fact that the target and source unit parameters were not in succession.

@PavelBal
Copy link
Member

Yeah we did not have the argument sourceUnit in the first implementation (it was always assumed to be in base unit. To not to break existing code, we decided not to change the order of the arguments.

@msevestre
Copy link
Member

yep that

@IndrajeetPatil
Copy link
Member Author

Hmm, so we need to live with this order for the rest of the eternity? Pity.

@msevestre
Copy link
Member

no I don't think so.
I think most people use named argument anyways
And if not, this will crash as the argument have different types
I am ok to change this

@msevestre
Copy link
Member

specifrically because we are introducing some braking changes

IndrajeetPatil added a commit that referenced this issue Mar 23, 2022
@PavelBal
Copy link
Member

@IndrajeetPatil Please also open an issue in esqlabs-r to check the code using toUnit.

@IndrajeetPatil
Copy link
Member Author

I checked all instances of usage, and nowhere does esqlabsR uses positional argument matching.

So this change doesn't break anything in esqlabsR! 🙌

@IndrajeetPatil
Copy link
Member Author

I also went through usage instances in the OSP organization, and none of them will break either!

msevestre pushed a commit that referenced this issue Mar 23, 2022
* change `toUnit` signature

closes #837

* tests

* Update NEWS.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants