Skip to content

Conversation

@vincenthsin
Copy link
Contributor

@vincenthsin vincenthsin commented Aug 5, 2019

fixes #90


This change is Reviewable

@vincenthsin vincenthsin changed the title fixes #90 rename ALL CAPS method name to camelCase Aug 5, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.856% when pulling 6d21ca9 on freeman0432:master into aa667bd on JavaMoney:master.

Copy link
Member

@keilw keilw left a comment

Choose a reason for hiding this comment

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

Thanks, I recall this was a rather static and constant instance, therefore the uppercase. @freeman0432 Are you a JCP member already? Especially for the API we can only merge PRs by JCP members.

@vincenthsin
Copy link
Contributor Author

Sorry, I am not JCP member. @keilw ,I didn't expect to bother you.

@keilw
Copy link
Member

keilw commented Aug 5, 2019

@freeman0432 Sorry to hear, you could join as Associate Member if you like, it is not hard to join and every membership (even for companies) is free, see https://jcp.org/en/participation/membership.
If you are not in the position to join the JCP, maybe @marschall could raise that PR for himself, because he already is a JCP member.

@stokito
Copy link
Member

stokito commented Aug 5, 2019

@keilw if the project have such limitation then it should be noted in CONTRIBUTE.md file
https://help.github.com/en/articles/setting-guidelines-for-repository-contributors

@keilw
Copy link
Member

keilw commented Aug 5, 2019

@stokito This is more or less common knowledge by all JSRs. As Jakarta EE with the former Java EE JSRs moves a large portion away, there are fewer "traditional" JSRs but e.g. https://github.com/jsr107/jsr107spec also does not have a HOWTO. We could create such document for a new release, right now the MR has other priorities.

@marschall
Copy link
Member

Thanks, I recall this was a rather static and constant instance, therefore the uppercase.

It is static but a method rather than a field, it should therefore not be ALL_CAPS.

@marschall
Copy link
Member

This does not fully fix #90. It fixes only one part of #90 namely the method naming that violates Java idioms. It does not remove the redundant null checks. I have no issues with the changes as long as

fixes #90

Is removed from the comment, otherwise GitHub closes #90 when it should not.

@keilw
Copy link
Member

keilw commented Aug 5, 2019

We also made exceptions where a method represents a constant value e.g. MetricPrefix.KILO() in JSR 385. In some cases a semantic meaning goes beyond the simple Java naming conventions.

@keilw
Copy link
Member

keilw commented Aug 5, 2019

Then let's close this one, @marschall or @stokito both of you are JCP members already, would you be able to propose another PR that fixes #90 completely?

@keilw keilw closed this Aug 5, 2019
@marschall
Copy link
Member

@keilw I submitted #125

@keilw
Copy link
Member

keilw commented Aug 5, 2019

@marschall Thanks, will check it out soon. Is there a behavioral change the BP might also need? The internals like uppercase I would not call so tragic there, but if it changes anything, that should also be backported.

@marschall
Copy link
Member

There is no behavioral change. These are private methods and do not impact any user. There might be slightly less Optional instances allocated but escape analysis may entirely remove them.
The lambdas itself are non-capturing and should not allocate.

@keilw
Copy link
Member

keilw commented Aug 5, 2019

Ok thanks, then it may be less relevant to backport them.

@vincenthsin
Copy link
Contributor Author

@keilw, thank you for your advice. I have joined the JCP

@keilw
Copy link
Member

keilw commented Aug 6, 2019

@freeman0432 Thanks a lot, we'll ask the PMO to add contributors who helped with the JSR until the MR1, if you want and your name is already listed in https://jcp.org/en/participation/members/ (it usually takes a bit after you submitted everything) we could also ask to add contributors. Otherwise everyone is welcom to join a follow-up JSR where contributors can also self-nominate (right now in Maintenance phase this is only possible by asking the PMO)

@vincenthsin
Copy link
Contributor Author

@keilw, excuse me! I have submitted my profile for a few days. But the note is "Profile information in JCP DB is updated after JCP PMO review." What should I do?

@keilw
Copy link
Member

keilw commented Aug 8, 2019

They will take a bit of time, no worries. Especially in Summer

@vincenthsin
Copy link
Contributor Author

@keilw, thanks a lot.

@vincenthsin
Copy link
Contributor Author

@keilw Help me.I send email to admin@jcp.org, no one reply me.

@keilw
Copy link
Member

keilw commented Aug 21, 2019

They seem to be on vacation, we submitted everything to finalize JSR 385 a little over a week ago and there was also no response. Plus Oracle does its house conference in less than a month, so probably not much activity before then ;-/

@vincenthsin
Copy link
Contributor Author

It turned out to be the case, thank you very much. Then I will wait for their holiday to end.

@vincenthsin
Copy link
Contributor Author

@keilw
I received an email notifying that the AMA has been completed, but my name is not listed at https://jcp.org/en/participation/members/.
Can you merge my PR now?

@keilw
Copy link
Member

keilw commented Aug 25, 2019

The PR is closed and has been done (the relevant parts) by others like @thodorisbais I believe.

@keilw
Copy link
Member

keilw commented Aug 25, 2019

If it hasn't could you forward the mail from PMO to werner@javamoney.org please?

@vincenthsin
Copy link
Contributor Author

Sorry, my expression is a bit problematic. I mean, can I submit a new RP the next time I contribute code?

@keilw
Copy link
Member

keilw commented Aug 26, 2019

Yes, if they have you in their system, it should be OK.

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.

Inconsistent null checks in Monetary

5 participants