Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ID Serial 2: change all IDs to CamelCase #4842

Closed
wants to merge 3 commits into from

Conversation

guoyuhong
Copy link
Contributor

What do these changes do?

Related issue number

Linter

  • I've run scripts/format.sh to lint the changes in this PR.

@guoyuhong guoyuhong requested a review from raulchen May 23, 2019 13:43
@guoyuhong
Copy link
Contributor Author

There is modin test fail, because the modin has used ray.ObjectID. Modin is another repo, what is the step to fix that?

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/906/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14373/
Test FAILed.

@robertnishihara
Copy link
Collaborator

Thanks @guoyuhong, do you prefer ObjectId because it is more consistent with the camelcase that we use in other places (e.g., Gcs)? ObjectID looks better to me since ID is typically capitalized (since it is an acronym).

Regarding the Modin issue, one option would be to comment out that test until it's changed in Modin and then update the version of Modin after that.

@guoyuhong
Copy link
Contributor Author

I prefer ObjectId a bit, because current Java lint tool require Java code to use ObjectId. That is to say, Java has different Id name than C++ and python.

@raulchen
Copy link
Contributor

Just quickly googled "camel case abbreviations". It seems that different languages and guides have different rules.
I don't have preference. But I think at least we should rename the class methods in id types to use camel case, e.g., UniqueID.FromRandom().

@guoyuhong
Copy link
Contributor Author

@raulchen OK, that will be done.

@guoyuhong
Copy link
Contributor Author

Close this PR and move the function CamelCase PR to #4896 .

@guoyuhong guoyuhong closed this May 30, 2019
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