nix-community / nix-ts-mode

An Emacs major mode for editing Nix expressions, powered by tree-sitter [maintainer=@remi-gelinas]
https://github.com/nix-community/nix-ts-mode
Other
50 stars 9 forks source link

fix: prepare for MELPA distribution #2

Closed purcell closed 1 year ago

purcell commented 1 year ago

See #1 :)

purcell commented 1 year ago

Hi Rémi! Any thoughts on this one?

purcell commented 1 year ago

I see some tests need attention, thanks for running this.

remi-gelinas commented 1 year ago

Oh, hey! Honestly I didn't even think this would be super useful to anybody, it's my first Emacs package and usage of elisp really. Which means, I don't have a great grasp on best practice for writing packages :D These fixes are awesome, thank you.

Looks like tests are failing against at least snapshot Emacs, any chance you can take a look @purcell ? I'm not sure if they are failing due to issues with the tests themselves, or changes in this PR.

I have some licensing questions, but I'll move them to #1. Thanks so much for the attention!

purcell commented 1 year ago

Oh, hey! Honestly I didn't even think this would be super useful to anybody, it's my first Emacs package and usage of elisp really. Which means, I don't have a great grasp on best practice for writing packages :D These fixes are awesome, thank you.

🙌

Looks like tests are failing against at least snapshot Emacs, any chance you can take a look @purcell ? I'm not sure if they are failing due to issues with the tests themselves, or changes in this PR.

It's strange, because I didn't obviously change anything that would affect the tests. I've added Emacs 29.1 to the CI matrix, and the tests pass for me locally at least. The workflows don't run in my fork because they're only configured for pull requests, so I haven't been able to replicate the failure there.

purcell commented 1 year ago

Very mysterious — 29.1 is failing in CI too. I made treesit print out its font-locking, and it indeed doesn't seem to be applying the expected bracket face. I can only imagine that the grammar being installed in CI is different from the one I have locally, and different from what the CI was previously picking up. You could perhaps re-run a recent Actions run on trunk to test that theory.

remi-gelinas commented 1 year ago

Very mysterious — 29.1 is failing in CI too. I made treesit print out its font-locking, and it indeed doesn't seem to be applying the expected bracket face. I can only imagine that the grammar being installed in CI is different from the one I have locally, and different from what the CI was previously picking up. You could perhaps re-run a recent Actions run on trunk to test that theory.

I don't have a recent-enough test run on trunk that is able to be re-run apparently. That said, I've taken off the option to require approval for forks to run workflows, so you should be able to freely run the workflows on your fork. I'll take a look in the meantime, maybe the grammar made some breaking changes.

purcell commented 1 year ago

Thanks. I was able to approve and run workflows in my fork even before this, which at least let me figure out that the CI results were different to my local results. Locally I'm using an Emacs from Nix, loaded with all the grammars:

                    emacs =
                      (self.emacsPackagesFor super.emacs29).emacsWithPackages (epkgs:
                        with epkgs; [
                          vterm
                          treesit-grammars.with-all-grammars
                        ]);
purcell commented 1 year ago

Okay, I figured it out. I'd removed the code that set treesit-font-lock-level (since we shouldn't override the user's preference) but in the tests I wasn't initially setting that in a way that the treesit initialisation would pick up — I was trying to set it locally after enabling the mode. Now I've figured it out, and the tests are passing again in all Emacs version. Sorry for the noise.

purcell commented 1 year ago

(What happened was that the default font lock level was being used during the tests, and that doesn't include font-locking brackets, for example.)

remi-gelinas commented 1 year ago

(What happened was that the default font lock level was being used during the tests, and that doesn't include font-locking brackets, for example.)

Ah, yes, I believe that's the reason I decided to set the user setting to 4 upon enabling the mode. I noticed that brackets weren't font-locked by default. Setting it before the tests run makes much more sense though.

I've licensed the package under GPLv3 in #5, but I believe the version bump it created conflicts with your keyword addition. I have a fixed branch ready but no push permissions to your fork - if you'd like to give me permission, I can push the conflict fix to your fork, or you can clean it up yourself if you prefer.

I've also added a PR title validation workflow in #4 to ensure squash commits are able to be parsed by the release workflow.

Otherwise, let's get this in, and I can open a PR to MELPA!

purcell commented 1 year ago

I added you as a collaborator on my fork, you might get an invite you need to accept first.

I can commit a recipe directly to MELPA when this is merged, since this is effectively pre-reviewed.