You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/05/22 03:31:23 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
thesmallstar opened a new pull request #923:
URL: https://github.com/apache/fineract/pull/923
We are using the following regex: ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
we are starting with the capital letter, I hope everybody agrees on that :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-637757609
I intentionally stopped on this one, since number of file changes would be more than 300 in this case. If so, merge conflicts will probably be too many to solve manually.
Was it a bad idea to stop this, did I get anything wrong?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar removed a comment on pull request #923: FINERACT-821 Made changes to enforce ConstantName Checkstyle (1)
Posted by GitBox <gi...@apache.org>.
thesmallstar removed a comment on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-638272823
@vorburger @xurror Can we merge this if build passes?
I tried to be cautious of what I am changing and not just find and replace, still a detailed review would really be great to reduce the chance of any error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-632947493
@vorburger is it okay to do this in pieces? Just that there are "too many" to tackle at once, I cannot directly replace them since it can be used as a variable(non-final) / function arg somewhere in some file.
So it takes particularly long time in this. I will not extend this for more than 3-4 days though(otherwise there might arise too many merge conflicts)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #923: FINERACT-821 Made changes to enforce ConstantName Checkstyle (1)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-638272823
@vorburger @xurror Can we merge this if build passes?
I tried to be cautious of what I am changing and not just find and replace, still a detailed review would really be great to reduce the chance of any error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #923: FINERACT-821 Made changes to enforce ConstantName Checkstyle (1)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-638274034
@vorburger @xurror Can we merge this if build passes?
I tried to be cautious of what I am changing and not just find and replace, still a detailed review would really be great to reduce the chance of any error. I tried not to change more than 25 files(unless very necessary) so the review is manageable :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-632997425
Yeah I don't mind if you raise several PRs re. this, and ONLY actually
enable the enforcement in your last one... but each individual one still
has to fully pass the build.
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-637749772
@thesmallstar don't forget about this one... almost 2 weeks old.
I'm assuming #943 won't do this automagically as well, right? Still worth finishing manually, then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-632690021
Yup, all upper-case for constants, as per [Checkstyle's ConstantName](https://checkstyle.sourceforge.io/config_naming.html#ConstantName), is a widely used de facto standard coding convention in Java. Let's do it! :smiling_imp:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-637797517
> I intentionally stopped on this one, since number of file changes would be more than 300 in this case. If so, merge conflicts will probably be too many to solve manually. Was it a bad idea to stop this, did I get anything wrong?
No, you're actually absolutely right, and raising a good point here. Hm, I'm not sure, how should we best do this? You suggested gradually, in a serious of several smaller PRs, right? So I guess what you could do is just remove actually already adding the `<module name="ConstantName" />` to `fineract-provider/config/checkstyle/checkstyle.xml` here (because it's not fully complete yet, right?), and rebase and un-draft this, and I'll merge it pronto (tomorrow), if it builds of course, and then you can keep doing that?
Then when only you are fully done, you raise a last PR for this which actually enforces `ConstantName`.
@xurror @awasum could also help to promptly merge (only) these kinds of PRs.
Sounds like a plan?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #923: FINERACT-821 Added and Enforced ConstantName checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #923:
URL: https://github.com/apache/fineract/pull/923#issuecomment-632624827
I think I need to explain in a better way what we are doing here :P
A constant is a static and final field, we are trying to define the regex that all constant names should fall under. If anyone doesn't have an objection we can go with the default format, i.e start with a capital letter, and do not allow underscores.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #923: FINERACT-821 Made changes to enforce ConstantName Checkstyle (1)
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #923:
URL: https://github.com/apache/fineract/pull/923
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org