openmrs-module-webservices.rest icon indicating copy to clipboard operation
openmrs-module-webservices.rest copied to clipboard

Published 20 hours ago • verified publisher icon openmrs

RESTWS-871: Follow naming convention for constants

Open docjon09 opened this issue 2 years ago • 11 comments

Description of what I changed

Changed names of constant variables to follow the naming convention

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-871

Checklist: I completed these to help reviewers :)

  • [x] My pull request only contains ONE single commit (the number above, next to the 'Commits' tab is 1).

    No? -> read here on how to squash multiple commits into one

  • [x] My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • [x] I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • [x] I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.

    No? -> execute above command

  • [x] All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • [x] My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

docjon09 avatar Apr 17 '22 10:04 docjon09

Is the ticket url correct?

kdaud avatar May 02 '22 13:05 kdaud

Is the ticket url correct?

oh sorry, it is not. Have changed it now.

docjon09 avatar May 02 '22 13:05 docjon09

Have you tried to check it out and confirmed based on the work in this PR?

I have updated the ticket link now.

docjon09 avatar May 02 '22 13:05 docjon09

What about the commit message?

kdaud avatar May 02 '22 13:05 kdaud

What about the commit message?

I can't change it now, right?

docjon09 avatar May 02 '22 13:05 docjon09

What about editing it to a preferred commit message? Don't you have the editing privilege on this PR?

kdaud avatar May 02 '22 13:05 kdaud

Isn't there any variable that has been left out?

kdaud avatar May 05 '22 23:05 kdaud

Isn't there any variable that has been left out?

I have capitalized the names of all variables with keyword 'static final'. But I have left the 'final' variables unchanged as some websites say that they need not be capitalized.

docjon09 avatar May 06 '22 07:05 docjon09

I have left the 'final' variables unchanged as some websites say that they need not be capitalized.

Could you share the link to the documentation?

kdaud avatar May 08 '22 09:05 kdaud

I have left the 'final' variables unchanged as some websites say that they need not be capitalized.

Could you share the link to the documentation?

https://softwareengineering.stackexchange.com/q/252243 The OpenMRS section for code convention tells that constants should be capitalized. But I wasn't sure whether final fields are considered constants as they are local to the class. The java documentation too doesn't mention whether final fields are to be named similarly or not. So, I left it out for now.

docjon09 avatar May 09 '22 13:05 docjon09

But I wasn't sure whether final fields are considered constants

Is this link of help? https://www.thoughtco.com/constant-2034049#:~:text=A%20constant%20is%20a%20variable,read%20and%20understood%20by%20others.

kdaud avatar May 09 '22 13:05 kdaud

Hey @docjon09, I noticed that there are some pending changes in this PR. Are there any updates on when it will be completed? Let me know if there's anything I can do to help move it forward.

jayasanka-sack avatar Jan 24 '23 15:01 jayasanka-sack

Hey @docjon09, I noticed that there are some pending changes in this PR. Are there any updates on when it will be completed? Let me know if there's anything I can do to help move it forward.

Sorry, I forgot about this. Yes, I require help here. According to https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html, all class constants should be capitalized. But I do not know whether 'final' variables are considered class constants as their values are different for different class instatiations. Also, here https://softwareengineering.stackexchange.com/a/252252, it says that constants named serialVersionUID and serialPersistentFields need not be capitalized. These names are there in this repo and have not been capitalized.

docjon09 avatar Jan 27 '23 07:01 docjon09

Hi @docjon09, Thank you for bringing this to our attention. You are correct that according to the Oracle Java Code Conventions, all class constants should be capitalized, but for instance final variables that are not static, Oracle does not have any specific guidelines on whether to use uppercase or lowercase letters.

Let's get this PR merged first. We apologize for the confusion and any inconsistencies in our naming conventions. I will make sure to review and update our docs accordingly to ensure consistency and compliance with industry standards. Thanks again for bringing this to our attention. ❤️

jayasanka-sack avatar Jan 27 '23 09:01 jayasanka-sack