search

jira-node / node-jira-client

A Node.js wrapper for the Jira REST API
https://jira-node.github.io/
MIT License
449 stars 168 forks source link

Consolidate addCommentAdvanced into addComment #319

Closed ayashjorden closed 3 years ago

ayashjorden commented 3 years ago

Fixes jira-node/node-jira-client/issues/259

ayashjorden commented 3 years ago

Hi @pioug , Appreciate your feedback on this one as its a priority for my team. The latest release does not include the addCommentAdvanced or the ability to pass addComment with comment options object.

Thank you

pioug commented 3 years ago

I am not sure to understand the goal of your PR. Is it just a refactoring? Seems like addCommentAdvanced can already achieve what you need. I supposed addComment has been preserved to avoid any breaking change so removing it now would be just troublesome.

addCommentAdvanced is included in the latest release and is documented https://jira-node.github.io/class/src/jira.js~JiraApi.html#instance-method-addCommentAdvanced 🤔

Can you explain again what you are aiming for?

ayashjorden commented 3 years ago

My change takes into account both input options (simple string comment and comment options), so its actually improves compatibility over having two methods that are doing the same. addCommentAdvanced might be ambiguous to the consumer (even though its documented).

I've updated my commit based on your feedback, LMK what you think.

pioug commented 3 years ago

I am not satisfied the API that you proposed. An argument that can be either a string or object is even more ambiguous than having addComment and addCommentAdvanced. So considering your change as an "improvement" is arguable.

Can you explain why is this a priority for your team? Maybe I miss something. From my point of view, it seems like nitpicking on method names. I can't reasonably accept PR every time someone is not happy with them.

I am not the original maintainer so I don't want to break the API for any reason. If I have too, it must really worth it.

ayashjorden commented 3 years ago

Probably missed the "advanced" method in vscode intellisense before I looked in the codebase, thought this would make things clearer.

If you're not the maintainer and don't feel comfortable with it, we can drop it since I have an advanced solution now. (pun partially intended). Thank you

pioug commented 3 years ago

Thank you very much understanding 👌 There is definitely something we could do to help people finding the method addCommentAdvanced more easily.