Skip to content

Conversation

@abelsiqueira
Copy link
Contributor

@abelsiqueira abelsiqueira commented Oct 3, 2024

Update the CLI at src/main.jl to allow running the checks.
The checks can be defined based on parsed arguments.
Create scripts/explicit-imports.jl to pass ARGS to the CLI when running pre-commit.
Create a pre-commit hook that runs the script.
Move the documentation of CLI from docs/api.md to the README.
Expands the CLI documentation with information on how to run the pre-commit hooks.

Closes #85


As I was finishing, I noticed that there is a src/main.jl file that is intended to be use as CLI.
The new script is intended to be used as a check tool, so the functionalities are mostly orthogonal.
If you prefer, I can try to merge the two.

As a second note, I haven't seen any place where you run ExplicitImports itself through ExplicitImports.
Let me know if that's on purpose, or if you'd like me to add a check using the script in the CI.
I believe ExplicitImports only pass 2 of the 7 checks, so I would exclude the others.

@ericphanson
Copy link
Member

As I was finishing, I noticed that there is a src/main.jl file that is intended to be use as CLI.
The new script is intended to be used as a check tool, so the functionalities are mostly orthogonal.
If you prefer, I can try to merge the two.

Yeah, we should merge these! src/main.jl should be the main CLI driver, the precommit hook should just invoke it.

However currently I am using the @main functionality which needs v1.12 (https://docs.julialang.org/en/v1.12-dev/manual/command-line-interface/#The-Main.main-entry-point). I think we can improve backwards compatibility by defining the function as main(...) instead of @main(...) and then doing

if @isdefined(var"@main")
    @main
end

at the end of the file. My understanding is @main just expands to main, but also opts-in to the entrypoint functionality, so we don't actually have to define the function as @main(...).

@ericphanson
Copy link
Member

The other thing is the CLI needs to be expanded to add the check_* functions if those are the ones you want to use. Currently the entrypoint just does the print_ one, which we should keep as the default, but we can use flags to do checks instead.

As a second note, I haven't seen any place where you run ExplicitImports itself through ExplicitImports.
Let me know if that's on purpose, or if you'd like me to add a check using the script in the CI.
I believe ExplicitImports only pass 2 of the 7 checks, so I would exclude the others.

This is on purpose :). I want to have bits that deliberately fail to have examples for the readme. Also, ExplicitImports is under development (although I've been busy), rather than in a "maintenance" point of its lifecycle, so I don't think at this point its lifecycle it's necessarily worth the tradeoff to use explicit imports everywhere.

@abelsiqueira
Copy link
Contributor Author

Great. I'll start merging the scripts and update the hook. The docs on the 1.12 scripts are only in the docs/src/api.md file. Any guidance on how you prefer to merge/update these with my new section in the README?

The tests seem to be broken from something else, by the way. Looks like some change in DataFrames.

@ericphanson
Copy link
Member

The docs on the 1.12 scripts are only in the docs/src/api.md file. Any guidance on how you prefer to merge/update these with my new section in the README?

Hm, I think we could move that section to the README, it's really a CLI rather than API. Then I think they could go together, usage from command line or usage as precommit hook.

@abelsiqueira abelsiqueira force-pushed the 85-pre-commit-hook branch 3 times, most recently from c1cb579 to 69ee353 Compare October 4, 2024 11:41
Update the CLI at src/main.jl to allow running the checks.
The checks can be defined based on parsed arguments.
Create scripts/explicit-imports.jl to pass ARGS to the CLI when running
pre-commit.
Create a pre-commit hook that runs the script.
Move the documentation of CLI from docs/api.md to the README.
Expands the CLI documentation with information on how to run the
pre-commit hooks.

Closes JuliaTesting#85
Copy link
Contributor Author

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

I've made the corrections and update to merge the CLI. I squashed and rebased the commits, updating the commit and PR description.
I haven't done any extensive testing, just copy-pasted the -m Module things for the other direct functionality.
Let me know your thoughts.

Comment on lines +4 to +6
# suppress warning about Base.parse collision, even though parse is never used
# this avoids a warning when loading the package while creating an unused explicit import
# the former occurs for all users, the latter only for developers of this package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this happened because of auto-running the formatter.

src/main.jl Outdated
str,
" See the output of `--help` for usage details.")
return 1
return exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently -m Module automatically translates the returned value to exit code, but for the Julia -e CLI execution, I had to use exit(1) instead.

@ericphanson
Copy link
Member

Thanks, this looks great. I took the liberty of pushing up some changes as my form of review, since I wanted to play around with it and try out the CLI changes.

  • I renamed cli back to main, since I think the -m functionality is intending to standardize main as the CLI entrypoint
  • I added exit(main(...)) as the way to invoke it manually. Otherwise, we call main twice on nightly, once manually and once automatically. I also removed all exit invocations from the src code; I really don't want any library usage of ExplicitImports to terminate the whole process, even if you do ExplicitImports.main(...).
  • I hid expected stacktraces from main calls with try-catches, since they don't point anywhere useful for the user. I decided not to wrap the whole main function body in a try-catch, bc that will make debugging hard if there are unexpected crashes.
  • I reorganized the README a little bit to put the CLI first and then refer to it for the precommit hooks. I think the CLI is more of a basic primitive and then precommit hooks are one way to use it.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 45.20548% with 40 lines in your changes missing coverage. Please review.

Project coverage is 93.36%. Comparing base (911b915) to head (ffeebdc).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/main.jl 45.20% 40 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   96.84%   93.36%   -3.49%     
==========================================
  Files           9       10       +1     
  Lines        1015     1175     +160     
==========================================
+ Hits          983     1097     +114     
- Misses         32       78      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ericphanson ericphanson changed the title Create script and pre-commit hook Expand CLI functionality in main, make available on earlier Julia versions, and add .pre-commit-config.yaml Oct 6, 2024
@abelsiqueira
Copy link
Contributor Author

Thanks for the changes and thanks for the detailed explanation. I'm really happy with the result.

@ericphanson ericphanson merged commit 24b0de3 into JuliaTesting:main Oct 6, 2024
5 of 6 checks passed
@abelsiqueira abelsiqueira deleted the 85-pre-commit-hook branch October 7, 2024 06:44
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.

Consider adding a pre-commit hook

3 participants