Skip to content

Renamed getStart => getStartPosition for consistency. #156

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
Closed
wants to merge 1 commit into from
Closed

Renamed getStart => getStartPosition for consistency. #156

wants to merge 1 commit into from

Conversation

dantleech
Copy link
Contributor

Renamed Node#getStart to Node#getStartPosition to be consistent with Node#getEndPosition.

@dantleech
Renamed getStart => getStartPosition for consistency.
7e57663 
- Renamed getStart to getStartPosition to be consistent
  with getEndPosition.
@msftclas
Copy link

@dantleech,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@dantleech, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

felixfbecker
felixfbecker reviewed Jun 18, 2017
View reviewed changes
Copy link

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

This is a breaking change

src/Node.php
@@ -354,7 +354,7 @@ public function jsonSerialize() {
}

/**
* Get the end index of a Node.
* Get the end position of a Node.
Copy link

Choose a reason for hiding this comment

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

index (=offset) is more precise than position. Position can also mean (line, column).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method uses the term Position. Maybe we should change the API to get[Start|End]Offset?

Copy link
Member

@roblourens roblourens Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I've thought about this, 'position' is not ideal because the language server protocol uses it for line/col and the parser also has LineCharacterPosition. There are a ton of references to 'position' so it would be a pain to change, but either index or offset would be better for this.

@roblourens roblourens mentioned this pull request Aug 7, 2017
Closed
@roblourens
Copy link
Member

roblourens commented Aug 7, 2017 •
edited
Loading

I'm going to close this for the reasons in the comment above, but I still want to make these names consistent. I filed #166 to come up with better names.

@roblourens roblourens closed this Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roblourens roblourens roblourens left review comments

@felixfbecker felixfbecker felixfbecker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dantleech @msftclas @roblourens @felixfbecker