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

Re: Rewrite merlin-completion-at-point integration to be faster and better #1827

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
34 changes: 27 additions & 7 deletions .github/workflows/emacs-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,40 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
os:
- ubuntu-latest
ocaml-compiler:
- 5.2.x
emacs_version:
- '27.2'
- '28.2'
- '29.1'
- '29.3'

Choose a reason for hiding this comment

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

Suggested change
- '29.3'
- '29.4'

- snapshot
fail-fast: false # don't stop jobs if one fails
env:
EMACS_PACKAGE_LINT_IGNORE: ${{ matrix.package_lint_ignore }}
EMACS_BYTECOMP_WARN_IGNORE: ${{ matrix.bytecomp_warn_ignore }}
steps:
- uses: purcell/[email protected]
with:
version: ${{ matrix.emacs_version }}
- uses: purcell/[email protected]
voodoos marked this conversation as resolved.
Show resolved Hide resolved
with:
version: ${{ matrix.emacs_version }}

- uses: actions/checkout@v4
- name: Run tests
run: 'cd emacs && ./check.sh'
- uses: actions/checkout@v4

- name: Set-up OCaml ${{ matrix.ocaml-compiler }}
uses: ocaml/setup-ocaml@v3
with:
ocaml-compiler: ${{ matrix.ocaml-compiler }}

- name: Install dependencies
run: |
opam pin menhirLib 20201216 --no-action
opam install --yes ppx_string ppx_compare
opam install . --deps-only --with-test --yes

- name: Build and install
run: |
opam install . --yes

- name: Run tests
run: 'cd emacs && opam exec -- ./check.sh'
12 changes: 10 additions & 2 deletions emacs/check.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
# Adapted from https://github.com/purcell/package-lint/blob/master/run-tests.sh
EMACS="${EMACS:=emacs}"

NEEDED_PACKAGES="package-lint company iedit auto-complete"
NEEDED_PACKAGES="package-lint company iedit auto-complete compat"

ELS_TO_CHECK=*.el
# To reduce the amount of false positives we only package-lint files
# that are actual installable packages.
PKGS_TO_CHECK="merlin.el merlin-ac.el merlin-company.el merlin-iedit.el"
PKGS_TO_CHECK="merlin.el merlin-ac.el merlin-company.el merlin-iedit.el merlin-cap.el tests/merlin-cap-test.el"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right any more. From a packaging point of view, merlin, merlin-ac, merlin-company and merlin-iedit are separate packages, because their dependencies are different. Each is built into a separate MELPA package, for granular installation.

However, presumably the intention is that merlin-cap will be part of the base merlin package, since it doesn't depend on anything opinionated that ships separately from Emacs. So it wouldn't be appropriate to run package-lint on merlin-cap separately: instead, run package-lint on both merlin-cap.el and merlin.el together, with the package-lint-main-file variable set to merlin.el.

And merlin-cap-test.el doesn't need package-linting, because it's not shipped in the package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I did that in a72fd55.

There a still spurious merlin-cap.el: error: You should depend on (emacs "28.1") if you need string-search'.` even if we use the compat module.

merlin.el:477:0: error: `merlin/unmake-point' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin.el:608:0: error: `merlin/call' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin.el:1194:0: error: `merlin/display-in-type-buffer' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin-cap.el:10:0: error: Package-Requires outside the main file have no effect.
merlin-cap.el:341:13: error: You should depend on (emacs "28.1") if you need `string-search'.
merlin-cap.el:356:19: error: You should depend on (emacs "28.1") if you need `string-search'.
merlin-cap.el:376:18: error: You should depend on (emacs "28.1") if you need `string-search'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I was overdue to tag a new package-lint release, so this spurious message was due to the build picking up an old version. Once MELPA has rebuilt the new package-lint version this complaint should go away.


INIT_PACKAGE_EL="(progn \
(require 'package) \
Expand Down Expand Up @@ -50,3 +50,11 @@ EMACS_PACKAGE_LINT_IGNORE=1
--eval "(require 'package-lint)" \
-f package-lint-batch-and-exit \
${PKGS_TO_CHECK} || [ -n "${EMACS_PACKAGE_LINT_IGNORE:+x}" ]

# Run tests in batch mode.
"$EMACS" -Q -batch \
--eval "$INIT_PACKAGE_EL" \
-L . \
--eval "(progn\
(load-file \"tests/merlin-cap-test.el\")\
(ert-run-tests-batch-and-exit))"
Loading
Loading