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

Position vs Point #5

Closed
muescha opened this issue Apr 24, 2020 · 5 comments · Fixed by #6
Closed

Position vs Point #5

muescha opened this issue Apr 24, 2020 · 5 comments · Fixed by #6
Labels
📚 area/docs This affects documentation 🗄 area/interface This affects the public interface ☂️ area/types This affects typings good first issue 👋 This may be a great place to get started! help wanted 🙏 This could use your insight or help 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on

Comments

@muescha
Copy link

muescha commented Apr 24, 2020

the readme is confusing.

the function expect an unist.Point instead of unist.Position

there should be this notation change for readme:

  • change Position to Point

PS: i think the refactor the function name toPosition to toPoint would be harder

@muescha muescha added 🙉 open/needs-info This needs some more info 🙋 no/question This does not need any changes labels Apr 24, 2020
@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 📚 area/docs This affects documentation 🔍 status/open 🗄 area/interface This affects the public interface 🙋 no/question This does not need any changes and removed 🙉 open/needs-info This needs some more info 🙋 no/question This does not need any changes labels Apr 24, 2020
@wooorm
Copy link
Member

wooorm commented Apr 24, 2020

Ah yeah, thanks. Reason is that we didn’t have a term to differentiate between the two when this project was created..

PR welcome to a) fix the docs, b) add a toPoint function and alias toPosition to it? Next major we could remove toPosition.

@ChristianMurphy ChristianMurphy added 🦋 type/enhancement This is great to have good first issue 👋 This may be a great place to get started! help wanted 🙏 This could use your insight or help 🙆 yes/confirmed This is confirmed and ready to be worked on and removed 🙋 no/question This does not need any changes 🔍 status/open labels Apr 24, 2020
@muescha
Copy link
Author

muescha commented Apr 24, 2020

declare namespace vfileLocation {
  type Position = Pick<Point, 'line' | 'column'>

//...
toOffset: (position: Position) => Offset

any hint how this would be changed?

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Apr 24, 2020

For a) do nothing.
For b)

declare namespace vfileLocation {
  type Point = Pick<unist.Point, 'line' | 'column'>
  /** @deprecated */
  type Position = Point
  //...
    toOffset: (position: Point) => Offset

@wooorm
Copy link
Member

wooorm commented May 19, 2020

@muescha What do you think, are you interesting and able to work on a PR?

@muescha
Copy link
Author

muescha commented May 20, 2020

it was out of my scope - i will give it a try

wooorm added a commit that referenced this issue Aug 21, 2020
- `toPosition` is considered deprecated

Closes GH-5.
@wooorm wooorm closed this as completed in #6 Aug 21, 2020
wooorm added a commit that referenced this issue Aug 21, 2020
Closes GH-5.
Closes GH-6.

Reviewed-by: Christian Murphy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 🗄 area/interface This affects the public interface ☂️ area/types This affects typings good first issue 👋 This may be a great place to get started! help wanted 🙏 This could use your insight or help 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

Successfully merging a pull request may close this issue.

3 participants