Skip to content

RESTWS-871: Follow naming convention for constants#549

Merged
dkayiwa merged 22 commits intoopenmrs:masterfrom
docjon09:RESTWS-871
Feb 28, 2023
Merged

RESTWS-871: Follow naming convention for constants#549
dkayiwa merged 22 commits intoopenmrs:masterfrom
docjon09:RESTWS-871

Conversation

@docjon09
Copy link
Contributor

@docjon09 docjon09 commented Apr 17, 2022

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 :)

  • 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

  • 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

  • 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

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

    No? -> execute above command

  • 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.

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

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

public abstract class ITBase {

private static final Object serverStartupLock = new Object();
private static final Object SERVER_STARTUP_LOCK = new Object();
Copy link
Member

Choose a reason for hiding this comment

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

@docjon09 are you sure serverStartupLock is a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't knew objects aren't considered variables. I have updated it now. Thanks.

* Update ITBase.java

* Update OpenmrsClassScanner.java
@kdaud
Copy link
Member

kdaud commented May 2, 2022

Is the ticket url correct?

@docjon09
Copy link
Contributor Author

docjon09 commented May 2, 2022

Is the ticket url correct?

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

@docjon09
Copy link
Contributor Author

docjon09 commented May 2, 2022

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

I have updated the ticket link now.

@kdaud
Copy link
Member

kdaud commented May 2, 2022

What about the commit message?

@docjon09
Copy link
Contributor Author

docjon09 commented May 2, 2022

What about the commit message?

I can't change it now, right?

@kdaud
Copy link
Member

kdaud commented May 2, 2022

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

@docjon09 docjon09 changed the title RESTWS-871: Refactor web-services module for code quality RESTWS-871: Follow naming convention for constants May 2, 2022
@kdaud
Copy link
Member

kdaud commented May 5, 2022

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

@docjon09
Copy link
Contributor Author

docjon09 commented May 6, 2022

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.

@kdaud
Copy link
Member

kdaud commented May 8, 2022

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?

@docjon09
Copy link
Contributor Author

docjon09 commented May 9, 2022

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.

@kdaud
Copy link
Member

kdaud commented May 9, 2022

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.

@jayasanka-sack
Copy link
Member

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.

@docjon09
Copy link
Contributor Author

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.

@jayasanka-sack
Copy link
Member

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. ❤️

@dkayiwa dkayiwa merged commit e00c95a into openmrs:master Feb 28, 2023
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.

4 participants