Skip to content

Creating an expandable enum type, converting StorageClass#1937

Merged
garrettjonesgoogle merged 3 commits into
googleapis:masterfrom
garrettjonesgoogle:alt
Apr 18, 2017
Merged

Creating an expandable enum type, converting StorageClass#1937
garrettjonesgoogle merged 3 commits into
googleapis:masterfrom
garrettjonesgoogle:alt

Conversation

@garrettjonesgoogle

@garrettjonesgoogle garrettjonesgoogle commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Design notes:

  • I made it so that converting an enum into a StorageEnumValue is not disruptive at a code level (e.g. it has valueOf and values, same as Java enums)
  • I am only converting one enum now, and after we agree on the design, I will do a follow-up PR to convert the rest.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 18, 2017
@garrettjonesgoogle

garrettjonesgoogle commented Apr 18, 2017

Copy link
Copy Markdown
Contributor Author

Updates #1832

shinfan
shinfan reviewed Apr 18, 2017
View reviewed changes

@shinfan shinfan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The design looks reasonable to me. Just a few comments.

Comment thread google-cloud-storage/src/main/java/com/google/cloud/storage/StorageClass.java
* Get the StorageClass for the given String constant, and throw an exception if the constant is
* not recognized.
*/
public static StorageClass valueOfStrict(String constant) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread google-cloud-core/src/main/java/com/google/cloud/StringEnumValue.java
public abstract class StringEnumValue {
private String constant;

@InternalApi

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread google-cloud-core/src/main/java/com/google/cloud/StringEnumType.java Outdated

public StringEnumType(String enumName, ApiFunction<String, EnumT> constructor) {
this.enumName = enumName;
this.constructor = constructor;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread google-cloud-core/src/main/java/com/google/cloud/StringEnumType.java Outdated

private final String enumName;
private final ApiFunction<String, EnumT> constructor;
private Map<String, EnumT> knownValues = new HashMap<>();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@michaelbausor

michaelbausor commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Design looks good to me too. Two points: (1) Is may be possible to push some functionality from StorageClass into StringEnumValue by making StringEnumValue parametrized recursively, e.g. class StringEnumValue<T extends StringEnumValue<T>>. This reduces the code in StorageClass, at the expense of more complexity. (2) Do we need tests for StringEnumValue and StringEnumType?

@garrettjonesgoogle

garrettjonesgoogle commented Apr 18, 2017

Copy link
Copy Markdown
Contributor Author

(1) Which functionality can be moved up? The only public things in StorageClass are static.
(2) It does make sense to make tests for StringEnumValue and StringEnumType - I'll add those in.

@coveralls

coveralls commented Apr 18, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 6db00d3 on garrettjonesgoogle:alt into ** on GoogleCloudPlatform:master**.

@michaelbausor

michaelbausor commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Re (1): you are right.

@garrettjonesgoogle

garrettjonesgoogle commented Apr 18, 2017

Copy link
Copy Markdown
Contributor Author

Added unit tests, PTAL

@coveralls

coveralls commented Apr 18, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling d6f19ab on garrettjonesgoogle:alt into ** on GoogleCloudPlatform:master**.

@coveralls

coveralls commented Apr 18, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling ded5357 on garrettjonesgoogle:alt into ** on GoogleCloudPlatform:master**.

@michaelbausor

michaelbausor commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

LGTM

@garrettjonesgoogle garrettjonesgoogle merged commit cc8dc45 into googleapis:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shinfan shinfan shinfan left review comments

@michaelbausor michaelbausor Awaiting requested review from michaelbausor

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@garrettjonesgoogle @michaelbausor @coveralls @shinfan @googlebot