-
-
Notifications
You must be signed in to change notification settings - Fork 134
assistant/supermaven-nvim: init #990
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
base: main
Are you sure you want to change the base?
Conversation
c7646cf
to
aa61ace
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
off the bat, that change will get it to build. On the rest, I'm unfamiliar with the workflow for Supermaven, but on the nix side (besides my reviews), things LGTM.
I recommend you use the checklist, regarding style and testing, that comes as the templates for new PRs :). It's great for first-time contributors to dodge "obvious" reviews and for long-time contributors as an actual checklist of things to remember (it's also just a bunch of things we don't have to wonder if you did, which makes reviewing easier).
Thanks for the work <3
p.s. It looks like it works correctly, but I noticed it downloading a binary? Unsure of where that's being stored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the good work. I'm leaving two comments for the sake of consistency with the project styles. Soliprem's comments also apply.
Lastly, please add a changelog entry before we can merge this.
When I created the PR it didn't pre-fill the description with the template. Don't know why that happened! Thanks for your feedback in the face of this. Also, apologies for not properly testing, I should have known better. I need to stop making midnight PRs🫠 |
42b53c7
to
8cd5020
Compare
I changed it to |
8cd5020
to
a63e3e1
Compare
Reading the source code, it looks like this gets put in |
Sorry for the late response. This is a relatively new "convention" that we've started applying in light of some future refactor plans. There are plugins that do not adhere to the convention yet, as they have been added before we even discussed such a naming decision. It'll be easier for everyone if you name the options after the plugins :) |
I also noticed this. I'm always adding |
I'm quite positive that the pull request template is active by default. Unless, of course, GH has changed how it functions. Last I've checked the template is "prompted" by default, and it's usable in CLI tools like |
What's left to do here? |
can be merged with @Soliprem's approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1cd70f7
to
81adc4e
Compare
CI was failing due to an oversight when renaming from supermaven to supermaven-nvim. fix pushed. Also force-pushed with rebase to resolve conflict. |
I'm sorry I've caused a merge conflict again, if you could resolve it I can merge this right away |
81adc4e
to
c86a8a7
Compare
So strange that that should cause a conflict. Done! |
supermaven-nvim's README claims nvim-cmp integration is as simple as adding
"supermaven"
to the sources list, so I felt it unnecessary to add a separate option for that here. Let me know if that should be changed.Also, this is my first time contributing here, so sorry if there's anything that doesn't fit standards. I referred to the other assistant options when writing this.