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

implement sum helper as strict alias for add helper #614

Merged

Conversation

Mifrill
Copy link
Contributor

@Mifrill Mifrill commented Sep 24, 2021

closes #40

@Mifrill Mifrill force-pushed the issue#40/implement-sum-helper branch from a437c49 to 63cbe31 Compare September 24, 2021 05:16
@Mifrill Mifrill marked this pull request as ready for review September 24, 2021 05:17
@Mifrill
Copy link
Contributor Author

Mifrill commented Sep 24, 2021

Hi, @skeate back to your #40 (comment)

I still don't agree with adding a separate sum helper, especially if it's not just an alias to add. Having to check the docs all the time to remember which was which would be a pain.

I did a bit confused in fact that the documentation for the addon math does not include sum helper for a quite simple case like get the sum of two numbers, you know. After some search, I did find your #40 (comment) to realized that it exists actually.
I assume not only me:
https://github.com/search?q=ember+%22export+function+sum%22&type=Code
Even though the name add is a quite correct naming for the underneath logic, the name sum sounds more native from a human perspective.
I think it's nice to highlight it and have sum helper. It seems like, it's more about consistent naming, so what do you think about add strict alias?

@@ -0,0 +1 @@
export { default, sum } from 'ember-math-helpers/helpers/sum';
Copy link
Owner

Choose a reason for hiding this comment

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

@Mifrill I think we could just export from add here and be good to go. Something like this maybe?

Suggested change
export { default, sum } from 'ember-math-helpers/helpers/sum';
export { default, add as sum } from 'ember-math-helpers/helpers/add';

Copy link
Contributor Author

@Mifrill Mifrill Sep 24, 2021

Choose a reason for hiding this comment

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

@rwwagner90 well, in this way file addon/helpers/sum.js will came redundant, right? So, then we would need to customize documentation somehow that provided by ember-cli-addon-docs because comments in file addon/helpers/sum.js have auto-converts into the component with documentation.
However, the original proposal is to have the documented sum helper:
Selection_823
maybe it's better to keep it consistent with the documentation addon with no this requested change, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwwagner90 could you give an advice on how to customize documentation for this particular case?

@Mifrill Mifrill force-pushed the issue#40/implement-sum-helper branch from 7fc503b to 63cbe31 Compare September 25, 2021 06:44
@RobbieTheWagner RobbieTheWagner merged commit a012f13 into RobbieTheWagner:main Oct 19, 2021
@Mifrill Mifrill deleted the issue#40/implement-sum-helper branch October 19, 2021 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any plans to addsum helper?
2 participants