Hi Yev,
I agree that the changes proposed are in line with accepted Enum naming conventions. The current naming is consistent with the RDF terms, which follows an RDF naming convention rather than Java naming convention. I believe the enum is used to map to both the tag value and the RDF term. If we change the name of the enum, I believe it will break the RDF term mappings (but I have not looked at the code for this in a while, I may be wrong). In terms of encapsulating the enum literal, I agree that this is an issue and should be addressed. We would need to have the ability to map between the Java enum value, the RDF term and the Tag. Gary From: [email protected] [mailto:[email protected]] On Behalf Of Yev Bronshteyn Sent: Wednesday, June 24, 2015 4:30 PM To: [email protected] Subject: Making enums consistent with conventions and best practices There are some issues in how enums are used in SPDX Tools. First, there are the names: RelationshipType.relationshipType_contains. The established convention (See “Effective Java”, Item 30 or Google’s style guide: 4.8.1 Enum classes <http://google.github.io/styleguide/javaguide.html#s4.8.1-enum-classes> , or just about anywhere else) is to have the values in upper case and not duplicate the enum name. Further, any literal data that is associated with enum constants should be contain within the enum. Today, we expose the mappings of enum values to literals in public mutable structures, violating encapsulation in a way that allows a subcomponent in the classpath to break how the code works in the entire application by changing those values. I would therefore propose rewriting the Relationship, Annotation, and File types in the following manner, consistent with universally-accepted Java programming practices: public enum RelationshipType { DESCRIBES("DESCRIBES"), DESCRIBED_BY("DESCRIBED_BY"), ANCESTOR_OF("ANCESTOR_OF"), BUILD_TOOL_OF("BUILD_TOOL_OF"), CONTAINED_BY("CONTAINED_BY"), CONTAINS("CONTAINS"), COPY_OF("COPY_OF"), DATA_FILE_OF("DATA_FILE_OF"), DESCENDANT_OF("DESCENDANT_OF"), DISTRIBUTION_ARTIFACT("DISTRIBUTION_ARTIFACT"), DOCUMENTATION_OF("DOCUMENTATION_OF"), DYNAMIC_LINK("DYNAMIC_LINK"), EXPANDED_FROM_ARCHIVE("EXPANDED_FROM_ARCHIVE"), FILE_ADDED("FILE_ADDED"), FILE_DELETED("FILE_DELETED"), FILE_MODIFIED("FILE_MODIFIED"), GENERATED_FROM("GENERATED_FROM"), GENERATES("GENERATES"), METAFILE_OF("METAFILE_OF"), OPTIONAL_COMPONENT_OF("OPTIONAL_COMPONENT_OF"), OTHER("OTHER"), PACKAGE_OF("PACKAGE_OF"), PATCH_APPLIED("PATCH_APPLIED"), AMENDS("AMENDS"), STATIC_LINK("STATIC_LINK"), TEST_CASE_OF("TEST_CASE_OF"); private final String tag; // Immutable map to look up values by tag private static final Map<String, RelationshipType> typesByTag; static { ImmutableMap.Builder<String, RelationshipType> builder = ImmutableMap.builder(); for (RelationshipType type : RelationshipType.values()) { builder.put(type.getTag(), type); } typesByTag = builder.build(); } private RelationshipType(String tag) { this.tag = tag; } public String getTag() { return tag; } public static RelationshipType fromTag(String tag) { return typesByTag.get(tag); } } Because this is a change to the API, I would love to make this change as soon as possible, perhaps in a separate point release, so that any new users will get the updated version. Yev Bronshteyn Senior Software Engineer Black Duck Software Black Duck Software <http://www.blackducksoftware.com/> | OpenHUB <https://www.openhub.net/> | OSDelivers <http://osdelivers.blackducksoftware.com/> | OSS Logistics <https://www.blackducksoftware.com/oss-logistics> <http://twitter.com/black_duck_sw> <https://www.linkedin.com/company/black-duck-software> <https://www.facebook.com/BlackDuckSoftware> <https://plus.google.com/+Blackducksoftware/> <http://www.slideshare.net/blackducksoftware> <https://www.youtube.com/user/BlackDuckSoftware> JP Morgan Chase & Co. Hall of Innovation Inductee
_______________________________________________ Spdx-tech mailing list [email protected] https://lists.spdx.org/mailman/listinfo/spdx-tech
