noctuid / general.el

More convenient key definitions in emacs
GNU General Public License v3.0
986 stars 43 forks source link

Allow :general use-package to be given multiple times #496

Closed mohkale closed 2 years ago

mohkale commented 2 years ago

Closes #446

codecov-commenter commented 2 years ago

Codecov Report

Merging #496 (defc11e) into master (a78da8f) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #496   +/-   ##
=======================================
  Coverage   69.40%   69.40%           
=======================================
  Files           1        1           
  Lines        1000     1000           
=======================================
  Hits          694      694           
  Misses        306      306           
Impacted Files Coverage Δ
general.el 69.40% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a78da8f...defc11e. Read the comment docs.

mohkale commented 2 years ago

@noctuid

I'm not sure why one of the test-runs is failing but I don't think it's because of the PR. Could you please reveiw?

mohkale commented 2 years ago

Please squash once you've made these changes

I'll try doing this now. Might've to rebase first since there's a merge in the middle :cry:.

mohkale commented 2 years ago

@noctuid

I ended up exporting everything as patches, rolling back to master, pulling in origin/master and then applying the patches on top. Out of curiosity do you know of a better way to do this? Either way now there's a single commit that has no conflicts with the base branch. It should be ready to merge once the CI/CD tests finish.

noctuid commented 2 years ago

I ended up exporting everything as patches, rolling back to master, pulling in origin/master and then applying the patches on top. Out of curiosity do you know of a better way to do this? Either way now there's a single commit that has no conflicts with the base branch

I'm not sure what the best thing to do in this situation is. I would have rebased on master instead of merging master into my feature branch. If the merge causes issues with rebase afterwards, probably creating one single patch vs. master is the easiest thing to do.

It should be ready to merge once the CI/CD tests finish.

It looks like it now has the old autoload parsing code instead of the code from master, so the tests are failing.

Could you also add something like this around line 883?

(it "should work with multiple :general statements"
    (use-package some-package
      :ensure nil
      :general
      (:keymaps 'general-temp-map
       "a" #'a)
      :general
      ("b" "b" :keymaps 'general-temp-map)
      :general
      ("c" #'c :keymaps 'general-temp-map)
      :general
      (general-temp-map "d" #'d)
      :general
      ('normal general-temp-map "e" #'e))
    (expect (general-test-keys nil general-temp-map
              "a" #'a
              "b" "b"
              "c" #'c
              "d" #'d))
    (expect (general-test-keys 'normal general-temp-map
              "e" #'e)))

Sorry for all the trouble. I would set the tests to autorun for this PR, but I don't see an option for that.

noctuid commented 2 years ago

Okay, I loosened the autorun restrictions, so hopefully the tests will automatically run from now on.

mohkale commented 2 years ago

@noctuid

So looks like there was a recent change to general (the :no-autoload option) which I accidentally ended up removing during the merge from upstream, which caused the tests to fail. I've re-added it in and all the tests are passing so let me know if there's anything else that needs to be done to merge. :smile:.

noctuid commented 2 years ago

Great, thanks!