org.hl7.fhir.core icon indicating copy to clipboard operation
org.hl7.fhir.core copied to clipboard

Names of Inner Classes in Consent Start With Lowercase Letter

Open hhund opened this issue 2 years ago • 3 comments

The names of the inner classes provisionComponent provisionActorComponent provisionDataComponent of Consent violate the Java naming conventions. See Java 11 SE Language Specification §6.1 [emphasis added]:

Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed case with the first letter of each word capitalized.

https://github.com/hapifhir/org.hl7.fhir.core/blob/45efd0afaf3c03bebbafafb11d44eebef2ac02d5/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/model/Consent.java#L1011

https://github.com/hapifhir/org.hl7.fhir.core/blob/45efd0afaf3c03bebbafafb11d44eebef2ac02d5/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/model/Consent.java#L1918

https://github.com/hapifhir/org.hl7.fhir.core/blob/45efd0afaf3c03bebbafafb11d44eebef2ac02d5/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/model/Consent.java#L2149

This issue is related to the "wont fix" close issue #403 and fixing the lowercase first letters p would be a breaking code change. I would suggest only fixing this via a major version release.

hhund avatar Jun 23 '22 15:06 hhund

@jamesagnew when do you think we should fix this?

grahamegrieve avatar Jul 19 '22 20:07 grahamegrieve

To be honest I don't think we should fix this, at least not right now.

The casing on these classes is inconsistent with the Java style guide and is likely to be at least a bit confusing to new users for sure, but it does not cause any compile errors or prevent use of the classes in any way that I am aware of (@hhund please let us know if you disagree with this statement).

On the other hand, fixing this is certain to cause a massive amount of headache to any developers who are relying on the existing casing. My suggestion would be to WONTFIX this for now, but revisit with the broader community for opinions on whether we should fix it the next time we do a major version bump.

jamesagnew avatar Aug 03 '22 11:08 jamesagnew

In my opinion, this issue is primarily an inconvenience. Static code analysis tools may have a problem with this tho'. While reading code that uses these classes, it took me a while to figure out that there was no bug in the code, but just a violation of naming conventions.

As a user of a library, I always assume that there might be breaking changes from one major version to the next. If the previous major version is still supported in parallel for some time and I am not forced to update immediately, this is not a problem.

It would have been nice if this had been fixed with version 6, but I guess I noticed this too late. I would suggest fixing this issue and perhaps making other breaking changes in version 7.

In general, HAPI and the FHIR core libraries should have a mechanism to even fix problems that lead to breaking code changes. Otherwise, the amount of technical debt that accumulates over time will increase the likelihood of having to start over at some point.

hhund avatar Aug 04 '22 11:08 hhund