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

Rework currencies #51

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Conversation

MrIvanPlays
Copy link
Member

Closes #49
Closes #40

@MrIvanPlays
Copy link
Member Author

@MrNemo64 @Geolykt

@lokka30
Copy link
Member

lokka30 commented Dec 6, 2021

Thank you very much! Only a few things I could point out to change, the rest is marvelous.

@lokka30
Copy link
Member

lokka30 commented Dec 6, 2021

Would like to hear from @Jikoo @MrNemo64 @Geolykt, if possible. Would be much appreciated :)

@lokka30
Copy link
Member

lokka30 commented Dec 6, 2021

Once we've gathered enough feedback, is this PR ready to merge as-is?

@MrIvanPlays
Copy link
Member Author

Once we've gathered enough feedback, is this PR ready to merge as-is?

Yes.

Copy link
Collaborator

@Jikoo Jikoo left a comment

Choose a reason for hiding this comment

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

Barring the potential annotation-related changes, looks good to me 👍

@lokka30
Copy link
Member

lokka30 commented Dec 6, 2021

I'll leave about a day until I merge it in case @Geolykt or anyone else wants to review the PR, also leaves more time in case you find an issue that needs to be addressed.

@MrIvanPlays
Copy link
Member Author

@lokka30 Happy now!?

@MrNemo64
Copy link
Contributor

MrNemo64 commented Dec 7, 2021

If we rename one of the methods that remove currencies by id, shouldn't all the methods that remove by id also change to match the style of the name? I know is not really necessary but to keep consistency

@MrIvanPlays
Copy link
Member Author

If we rename one of the methods that remove currencies by id, shouldn't all the methods that remove by id also change to match the style of the name? I know is not really necessary but to keep consistency

And I think to myself........ Why the fuck

@lokka30
Copy link
Member

lokka30 commented Dec 7, 2021

@lokka30 Happy now!?

They were just very minor nitpicks, I was happy to begin with :)
Will merge now.

If we rename one of the methods that remove currencies by id, shouldn't all the methods that remove by id also change to match the style of the name? I know is not really necessary but to keep consistency

And I think to myself........ Why the fuck

Consistency can be quite important, we should apply it wherever we can. If it's unfeasible for us to modify something to strive for better consistency then that's completely fine. MrNemo said it wasn't important so we should focus on the next things to do for Treasury instead of trying to iron out every single detail for the moment

@lokka30 lokka30 merged commit ecc89d1 into ArcanePlugins:master Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entirely rework currencies Parsing sums of money
4 participants