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.

Sign up for GitHub

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

Jump to bottom

Gizmo: make methods lower case #822

Merged
merged 14 commits into from
Sep 22, 2019
Merged

Gizmo: make methods lower case #822

merged 14 commits into from
Sep 22, 2019

Conversation

iddan
Copy link
Collaborator

@iddan iddan commented Sep 15, 2019 •
edited by dennwc
Loading

This change is Reviewable

@iddan iddan requested a review from dennwc as a code owner September 15, 2019 19:25
@iddan iddan mentioned this pull request Sep 15, 2019
Open
dennwc
dennwc requested changes Sep 15, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Thanks a lot for starting the effort to make Gizmo compatible with Gremlin spec! The change looks good, but it may only target v0.8 if it breaks compatibility. But it's not ideal to wait for it, so instead we can do the following:

query/gizmo/gizmo.go Outdated Show resolved Hide resolved
query/gizmo/gizmo_test.go Outdated Show resolved Hide resolved
@dennwc dennwc added this to the v0.7.6 milestone Sep 15, 2019
@iddan
Copy link
Collaborator Author

iddan commented Sep 17, 2019 •
edited
Loading

Made requested changes, renamed g.Uri to g.IRI as it is a more correct name and this is a breaking change anyway

@iddan
Copy link
Collaborator Author

iddan commented Sep 18, 2019

@dennwc I think it's ready for another review.

@dennwc
Copy link
Member

dennwc commented Sep 18, 2019

Sorry for not being specific enough, but can you please make the same change and allow other methods as well, so we don't break any old clients, but will still introduce a lower-case names as an alternative?

@iddan
Copy link
Collaborator Author

iddan commented Sep 18, 2019

All relevant methods are now accessible capitalized, and not.

dennwc
dennwc requested changes Sep 18, 2019
View reviewed changes
query/gizmo/gizmo.go Show resolved Hide resolved
@iddan iddan changed the base branch from master to fix-recursive September 22, 2019 01:27
@iddan iddan changed the base branch from fix-recursive to master September 22, 2019 01:27
@iddan
Copy link
Collaborator Author

iddan commented Sep 22, 2019

Resolved conflicts

dennwc
dennwc approved these changes Sep 22, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

LGTM after CI tests pass.

GH still complains about conflicts, so squashing/rebasing may be a good idea.

@iddan iddan merged commit 39953e0 into cayleygraph:master Sep 22, 2019
@iddan iddan deleted the lowercase-methods branch September 22, 2019 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennwc dennwc dennwc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.7.6
Development

Successfully merging this pull request may close these issues.
2 participants
@iddan @dennwc