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

2. Milestone: Internal Cleanup #87

Merged
merged 16 commits into from
Dec 1, 2024
Merged

Conversation

mekeor
Copy link
Collaborator

@mekeor mekeor commented Nov 24, 2024

This PR fixes all issues under milestone 2 - internal cleanup. It aims to clean up some internals while avoiding breaking changes.

Each commit solves a specific problem. The PR can be best reviewed by reviewing each commit on its own.

@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch 10 times, most recently from 6862cd2 to fbd1cb2 Compare November 28, 2024 00:15
Fixes issue #73 "Add copyright notes for future changes" #73
- Fix URL from leanprover/lean4-mode to
  leanprover-community/lean4-mode.
- Default use-package :vc approach to :rev :last-release.
- Use "major mode" rather than "major-mode".
- Add .dir-locals.el
- Describe change in README.org, chapter "Contributing"
@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch from fbd1cb2 to ca2037a Compare November 30, 2024 22:35
@mekeor mekeor requested a review from urkud November 30, 2024 22:42
@mekeor mekeor marked this pull request as ready for review November 30, 2024 22:56
@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch from 62706f3 to 7048820 Compare November 30, 2024 23:02
@urkud
Copy link
Member

urkud commented Dec 1, 2024

Could you please also fix the long lines warnings from melpazoid?

@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch from 8ac148b to 814010b Compare December 1, 2024 10:44
@mekeor
Copy link
Collaborator Author

mekeor commented Dec 1, 2024

@urkud, are you referring to this?:

The ;;; Commentary for this file is much wider than 80 characters

Let's take a look at each of these:


lean4-eri.el with melpazoid:

- The `;;; Commentary` for this file is much wider than 80 characters

This refers to this URL in the commentary: https://github.com/agda/agda/blob/b40c6fc2e0ced7b547553654f81e5898082d700c/src/data/emacs-mode/eri.el. But I'd prefer to keep it as is.


lean4-info.el with byte-compile using Emacs 29.4:

lean4-info.el:75:2: Warning: docstring wider than 80 characters

Fixed in a new commit: 89a1b29


lean4-input.el with melpazoid:

- The `;;; Commentary` for this file is much wider than 80 characters

This refers to the URL https://github.com/agda/agda/blob/d2cbd2dd4f49fa84c5ca6fcf464c3211adcc0088/src/data/emacs-mode/agda-input.el in the commentary which I'd also like to keep as is.

@mekeor
Copy link
Collaborator Author

mekeor commented Dec 1, 2024

Generally, I dislike Melpazoid. It's implemented in Python and has too many false-negatives. Its author themself admits this too. We may look for an alternative some time in future.

@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch 4 times, most recently from 9f048d2 to 3248ed4 Compare December 1, 2024 23:30
@mekeor
Copy link
Collaborator Author

mekeor commented Dec 1, 2024

I was mistaken. This...:

lean4-info.el with byte-compile using Emacs 29.4:

lean4-info.el:75:2: Warning: docstring wider than 80 characters

... has not been fixed by one of my commits. Instead, it is referring to this...:

(eval-when-compile
  (lsp-interface
    (lean:PlainGoal (:goals) nil)
    (lean:PlainTermGoal (:goal) nil)
    (lean:Diagnostic (:range :fullRange :message) (:code :relatedInformation :severity :source :tags))))

which expands to a very long Elisp code including this definition that Melpazoid may be warning about due to a too wide docstring:

(cl-defun lsp-make-lean-diagnostic (&rest plist &key code? related-information? severity? source? tags? range full-range message &allow-other-keys) (ignore code? related-information? severity? source? tags? range full-range message) "Constructs lean:Diagnostic from `plist.'
Allowed params: (:message :full-range :range :tags? :source? :severity? :related-information? :code?)"
  ;; ...
  )

Long story short, this is an issue in lsp-mode because they are the developers of the lsp-interface macro.

mekeor and others added 4 commits December 2, 2024 00:53
Also use add-to-list so that re-evaluating the file won't add another
entry to auto-mode-alist.
Fixes #69 "Don't depend on Flycheck":
#69

- Add some minor comments in code
- Remove Flycheck as dependency in Package-Requires library header
- Adjusted README accordingly
Rephrase some single-line summaries.

Use semicolon in prop-line as `add-file-local-variable-prop-line`
does.

Remove all library headers except for lean4-mode.el.

Put "This file is not part of GNU Emacs." comment in all file headers
consistently.

Consistently use same licensing comment in all files while adhering to
default fill-column.

In lean4-eri.el and lean4-input.el, put the copyright notice from the
original file at Agda-Mode.  For this, I determined the exact commits
from which Lean4-Mode (actually, originally Lean3-Mode, formerly
called Lean-Mode) derived.  I used the copyright notice from the Agda
repository's LICENSE file at that commit.  These commits are also
named as part of a mentioned Github-URL.

Make lean4-input.el properly end in "(provide 'lean4-input)\n;;;
lean4-input.el ends here" rather than these two lines occurring
somewhere in between.
This is just to silence Melpazoid warnings.
This call compatible with the signature of both emacs<30 and emacs>=30
because even though the signature of `derived-mode-p` changed, it
still allows for the now single argument to be a symbol, not just a
list of symbols.
- Remove weird comment ";; Automode List".
- For lean4-mode, use a docstring starter similar to other places.
- In lean4-mode docstring, drop "Invokes `lean4-mode-hook'." because
  Emacs will add information about the hook automatically anyway.
- Avoid (add-hook 'lean4-mode-hook #'lsp) as Melpazoid warns about it.
  Instead, define lean4-mode-hook explicitely with `defcustom` before
  the mode is defined. `lsp` being its only default member (for now).
  Also offer flycheck-mode as option for users of Customize.  The
  defcustom is placed in lean4-settings.el (for now) because that's
  where most user-options are defined.
@mekeor mekeor force-pushed the milestone-02-internal-cleanup branch from 3248ed4 to a4a47bb Compare December 1, 2024 23:53
@mekeor mekeor merged commit f0c7358 into master Dec 1, 2024
3 checks passed
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.

2 participants