Hi Yev,
Good points. Responses inline below. Gary From: Yev Bronshteyn [mailto:[email protected]] Sent: Thursday, June 25, 2015 7:18 AM To: Gary O'Neall Cc: [email protected] Subject: Re: Making enums consistent with conventions and best practices The relationship names in the RDF spec do not include the prefix “relationshipType_”. If something else requires mapping the term [Gary] It does include the prefix relationshipType_ (see the relationship type descriptions in http://spdx.org/rdf/terms/index.html). If the SPDX specification documentation is inconsistent or not clear, we should file a bug to address. The RDF naming convention for all individuals have a prefix to make sure they are unique within the SPDX namespace. In the SPDX tools code we took the approach of mapping the individuals to enums. “relationshipType_amends” to the value “AMENDS”, then we should add another static factory method e.g. “public static RelationshipType fromRdfTerm(String rdfTerm)” that produces that mapping and another accessor that provides the RDF term value. [Gary] We could take this approach, but the convention used in the SPDX tools up until now was to use the RDF term for the Enum name. It was a conscious decision to violate the Java naming conventions for Enums in these cases. We don’t have to choose between Java convention or the RDF convention. The java code can follow the Java convention without violating what we read from/write to RDF> [Gary] Agree with your statement, but we would want to take a consistent approach across the SPDX tools which would create compatibility issues with the other enums since they are types used in public method calls. I would propose we continue the same convention of using the RDF term names for the Enums even though it does not follow the Java naming conventions. I could document this in the code to make it clear that this is a conscious decision. On Jun 24, 2015, at 9:35 PM, Gary O'Neall <[email protected]> wrote: 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: <mailto:[email protected]> [email protected] [ <mailto:[email protected]> mailto:[email protected]] On Behalf Of Yev Bronshteyn Sent: Wednesday, June 24, 2015 4:30 PM To: <mailto:[email protected]> [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: <http://google.github.io/styleguide/javaguide.html#s4.8.1-enum-classes> 4.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. <image001.png> Yev Bronshteyn Senior Software Engineer Black Duck Software <http://www.blackducksoftware.com/> Black Duck Software | <https://www.openhub.net/> OpenHUB | <http://osdelivers.blackducksoftware.com/> OSDelivers | <https://www.blackducksoftware.com/oss-logistics> OSS Logistics <http://twitter.com/black_duck_sw> <image002.png> <https://www.linkedin.com/company/black-duck-software> <image003.png> <https://www.facebook.com/BlackDuckSoftware> <image004.png> <https://plus.google.com/+Blackducksoftware/> <image005.png> <http://www.slideshare.net/blackducksoftware> <image006.png> <https://www.youtube.com/user/BlackDuckSoftware> <image007.png> JP Morgan Chase & Co. Hall of Innovation Inductee 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
