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

feat: support "prio" priority value for gutter marks #43

Merged
merged 12 commits into from
Jun 2, 2024

Conversation

josh-nz
Copy link
Contributor

@josh-nz josh-nz commented May 29, 2024

This is my first pass at implementing #42.

Closes #42

I have not adjusted nor added any tests and therefore these might be broken. As per my previous pull request, my understanding of how to implement and run Neovim plugin tests is insufficient for me to be useful in this area.

@tris203 tris203 changed the title feat: support "prio" priorty value for gutter marks feat: support "prio" priority value for gutter marks May 29, 2024
Copy link
Owner

@tris203 tris203 left a comment

Choose a reason for hiding this comment

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

Couple of points:

  • In the default config we should add priorities, shorter movements should have higher priority. I would suggest, G = 10, gg = 9, { = 8, } = 8
    • This will break some tests but means we handle priority the same for gutters as we do for lines
    • We should add tests for priority replacing in gutter_hints_spec

If you need help with testing then more than happy to help guide you through it

But thanks for the PR @josh-nz. Its great to have help!

README.md Outdated Show resolved Hide resolved
lua/precognition/init.lua Outdated Show resolved Hide resolved
@tris203
Copy link
Owner

tris203 commented May 30, 2024

For some general help @josh-nz

The checks can be ran on your local machine by running make test

I really should write up a CONTRIBUTING.md

@josh-nz
Copy link
Contributor Author

josh-nz commented May 31, 2024

  • In the default config we should add priorities, shorter movements should have higher priority. I would suggest, G = 10, gg = 9, { = 8, } = 8

This was addressed in commit 88ab881 I believe.

  • We should add tests for priority replacing in gutter_hints_spec

Thanks for your help with the tests. I did figure out to run make to run the tests, and I was thinking of adding some notes about this and stylua to the readme (I was hoping to get away with manual formatting but my default nvim has different settings and I would sometimes forget to adjust them, and also pasting sometimes got it wrong, so I ended up downloading stylua). Would you prefer them in a separate MD file instead though?

Anything else that needs to be done?

@tris203 tris203 self-requested a review May 31, 2024 06:33
Copy link
Owner

@tris203 tris203 left a comment

Choose a reason for hiding this comment

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

I think this is ready to go, let me test it for a few days and then we will get it merged

Closes #42

@tris203 tris203 mentioned this pull request May 31, 2024
7 tasks
@josh-nz
Copy link
Contributor Author

josh-nz commented May 31, 2024

One detail not documented is what happens if there is a conflict between hints with the same priority. In the case of the gutter hints, I believe it is first-in wins, where first-in is defined by the internal gutter hints Lua table ordering (ie, not config). Not sure how relevant this is to be noted anywhere, but something to consider. Similarly for the line hints.

@tris203 tris203 force-pushed the feature/gutter-priority branch from 6ee2b32 to 308ba29 Compare June 1, 2024 10:01
@tris203 tris203 added this to the 1.0.0 milestone Jun 1, 2024
@tris203 tris203 force-pushed the feature/gutter-priority branch from 308ba29 to 6e7eb55 Compare June 1, 2024 22:04
@tris203
Copy link
Owner

tris203 commented Jun 1, 2024

One detail not documented is what happens if there is a conflict between hints with the same priority. In the case of the gutter hints, I believe it is first-in wins, where first-in is defined by the internal gutter hints Lua table ordering (ie, not config). Not sure how relevant this is to be noted anywhere, but something to consider. Similarly for the line hints.

@willothy what are your thoughts on this, I'm sort of thinking if you footgun yourself with your config raise an issue?

@josh-nz
Copy link
Contributor Author

josh-nz commented Jun 1, 2024

That's my preference. It's the simplest solution and doesn't add any additional code burden. Perhaps a quick note in the readme something to the effect of "make any hints that could appear in the same place as others have unique priorities to avoid conflicts"

@willothy
Copy link
Collaborator

willothy commented Jun 1, 2024

Yeah, I agree. I don't think ordering of same-priority items is particularly important, and if someone does run into an issue they can ask about it.

@tris203
Copy link
Owner

tris203 commented Jun 1, 2024

Excellent, if you want to add a note to the README @josh-nz then we can get this merged

@josh-nz
Copy link
Contributor Author

josh-nz commented Jun 1, 2024

README updated.

@willothy
Copy link
Collaborator

willothy commented Jun 2, 2024

I think @tris203 is asleep so I'm gonna go ahead and merge haha.

Thanks for all your work @josh-nz!

@willothy willothy merged commit cab9d0a into tris203:main Jun 2, 2024
9 checks passed
@josh-nz josh-nz deleted the feature/gutter-priority branch June 2, 2024 01:07
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.

Proposal - configurable horizontal and vertical motion display
3 participants