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

Fix dtrt-indent warnings #87

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Conversation

jamescherti
Copy link
Contributor

@jamescherti jamescherti commented Jan 29, 2025

Fix dtrt-indent warnings:

  • dtrt-indent.el:223:9: Warning: defvar ‘dtrt-indent-language-syntax-table’ docstring contains control char #x01 (position 629)
  • dtrt-indent.el:223:9: Warning: defvar ‘dtrt-indent-language-syntax-table’ docstring contains control char #x02 (position 671)
  • dtrt-indent.el:223:9: Warning: defvar ‘dtrt-indent-language-syntax-table’ docstring contains control char #x01 (position 629)
  • dtrt-indent.el:223:9: Warning: defvar ‘dtrt-indent-language-syntax-table’ docstring contains control char #x02 (position 671)
  • Warning (native-compiler): ~/src/forks/dtrt-indent/dtrt-indent.el:202:53: Warning: the function ‘smie-config-guess’ is not known to be defined.
  • dtrt-indent.el:206:19: Warning: reference to free variable ‘dtrt-indent-run-after-smie’
  • Add lexical-binding directive to the first line of dtrt-indent.el.
  • Declare a dependency on Emacs version x.y.
  • Include the project URL in the header.
  • Private functions (dtrt-indent--mode) cannot be autoloaded.

@jamescherti jamescherti changed the title Fix warnings Fix dtrt-indent warnings Jan 29, 2025
@rrthomas
Copy link
Collaborator

Amazing, many thanks! I have a few minor comments.

Copy link
Collaborator

@rrthomas rrthomas left a comment

Choose a reason for hiding this comment

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

Please don't require smie. dtrt-indent should not load packages that aren't already being used.

Copy link
Collaborator

@rrthomas rrthomas left a comment

Choose a reason for hiding this comment

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

Please squash this commit into the one that changed the URL in the first place.

- Add `lexical-binding` directive to the first line of `dtrt-indent.el`.
- Declare a dependency on Emacs version x.y.
- Include the project URL in the header.
- Private functions (`dtrt-indent--mode`) cannot be autoloaded.
- Warning (native-compiler):
  ~/src/forks/dtrt-indent/dtrt-indent.el:202:53: Warning: the function
  ‘smie-config-guess’ is not known to be defined.
- dtrt-indent.el:206:19: Warning: reference to free variable
  ‘dtrt-indent-run-after-smie’
- dtrt-indent.el:223:9: Warning: defvar
  ‘dtrt-indent-language-syntax-table’ docstring contains control char
  #x01 (position 629)
- dtrt-indent.el:223:9: Warning: defvar
  ‘dtrt-indent-language-syntax-table’ docstring contains control char
  #x02 (position 671)
- dtrt-indent.el:223:9: Warning: defvar
  ‘dtrt-indent-language-syntax-table’ docstring contains control char
  #x01 (position 629)
- dtrt-indent.el:223:9: Warning: defvar
  ‘dtrt-indent-language-syntax-table’ docstring contains control char
  #x02 (position 671)
@jamescherti
Copy link
Contributor Author

Amazing, many thanks! I have a few minor comments.

My pleasure, @rrthomas.

Please don't require smie. dtrt-indent should not load packages that aren't already being used.
Please squash this commit into the one that changed the URL in the first place.

I have resolved the issue by removing the dependency on smie and squashing the commits.

@rrthomas rrthomas merged commit 14fb127 into jscheid:master Jan 29, 2025
2 checks passed
@rrthomas
Copy link
Collaborator

Thanks for the quick reply! Merged. I'll tag a release.

@jamescherti
Copy link
Contributor Author

jamescherti commented Jan 29, 2025

Thanks for the quick reply! Merged. I'll tag a release.

Excellent!

For your reference, here are the remaining warnings that I did not address, as I prefer to leave the decision on how to handle them up to you:

  342   4 error    p-l-f    You should depend on (emacs "29.1") if you need `js-json-mode'.
  353   4 error    p-l-f    `ada-mode' was removed in Emacs version 27.1.
  386   4 error    p-l-f    You should depend on (emacs "29.1") if you need `c-ts-mode'.
  387   4 error    p-l-f    You should depend on (emacs "29.1") if you need `c++-ts-mode'.
  388   4 error    p-l-f    You should depend on (emacs "29.1") if you need `go-ts-mode'.
  390   4 error    p-l-f    You should depend on (emacs "29.1") if you need `java-ts-mode'.
  391   4 error    p-l-f    You should depend on (emacs "29.1") if you need `rust-ts-mode'.
  392   4 error    p-l-f    You should depend on (emacs "29.1") if you need `json-ts-mode'.
  393   4 error    p-l-f    You should depend on (emacs "29.1") if you need `cmake-ts-mode'.
  394   4 error    p-l-f    You should depend on (emacs "29.1") if you need `typescript-ts-base-mode'.

These warnings can be resolved by removing ada-mode from dtrt-indent-hook-mapping-list and updating Package-Requires: ((emacs "24.4")) to Package-Requires: ((emacs "29.1")). However, there may be a better solution to silence them.

@rrthomas
Copy link
Collaborator

rrthomas commented Jan 29, 2025

I think the best solution is to wait a few years (at which point it will be acceptable to bump the minimum version of Emacs). dtrt-indent does not strictly speaking need any of those modes; it would be pretty annoying if you had to install them all just to use dtrt-indent, let alone use a version of Emacs that supports them.

These are, after all, just warnings, not bugs, and in this (rather unusual) case, they're not useful.

Thus for example, the warning about ada-mode, now a 3rd-party package will never be addressed.

@jamescherti jamescherti deleted the fix-warnings branch January 29, 2025 19:03
@jamescherti
Copy link
Contributor Author

I understand your point, @rrthomas. I agree that waiting for a few years before bumping the minimum version of Emacs seems like a reasonable approach.

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