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

Wip tools lint-file #46

Merged
merged 5 commits into from
Mar 28, 2025
Merged

Wip tools lint-file #46

merged 5 commits into from
Mar 28, 2025

Conversation

mbarbin
Copy link
Owner

@mbarbin mbarbin commented Mar 27, 2025

This is an alternate to #40 where the command is named tools lint-file and will have an --in-place flag too.

Work in progress.

@mbarbin
Copy link
Owner Author

mbarbin commented Mar 27, 2025

@arvidj Please have a look when you get a chance. I made the changes we talked about in #40 to rename dunolint tools lint-file and have an --in-place flag.

I added tests and a changelog entry too, presumably we'll go with this PR instead if this checks out on your side. Thanks!

@mbarbin mbarbin added the enhancement New feature or request label Mar 27, 2025
@arvidj
Copy link

arvidj commented Mar 28, 2025

It works on the terminal as I'd expect. Works with Emacs reformatter with the updated definition:

(reformatter-define dunolint-format
  :program "dunolint"
  :args (list "tools" "lint-file" "--filename" (buffer-file-name))
  :lighter " DoF")

Works with pre-commit using the updated hook definition:

  - repo: local
    hooks:
      - id: dunolint
        name: Dunolint
        description: This hook runs dunolint on 'backend/sre/hydra'
        language: system
        entry: dunolint
        args: ["tools", "lint-file", "--in-place"]
        files: '*dune$' # To be refined...
        types: [text]

In sum, LGTM. If we settle on this version, I can make a separate PR to update the README.md with instructions for how to set up emacs and pre-commit for users that want that.

@mbarbin
Copy link
Owner Author

mbarbin commented Mar 28, 2025

Excellent, thanks a lot! I'm merging this.

By the way, beyond the use in editor setup, I also started finding usage for this function when writing tests and documentation, so I'm definitely happy to have this join the set of commands for the project.

PR for the readme sounds perfect to me, thanks a lot!

@mbarbin mbarbin merged commit a897419 into main Mar 28, 2025
3 checks passed
@mbarbin mbarbin deleted the lint-file branch March 28, 2025 09:52
@mbarbin mbarbin mentioned this pull request Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants