Skip to content

opam admin check: Set with-test and with-doc to false by default#6335

Merged
kit-ty-kate merged 1 commit intoocaml:masterfrom
kit-ty-kate:admin-check-test-doc-dev-setup
Jan 20, 2025
Merged

opam admin check: Set with-test and with-doc to false by default#6335
kit-ty-kate merged 1 commit intoocaml:masterfrom
kit-ty-kate:admin-check-test-doc-dev-setup

Conversation

@kit-ty-kate
Copy link
Copy Markdown
Member

@kit-ty-kate kit-ty-kate commented Dec 10, 2024

Queued on top of #6331

Currently opam admin check sets with-test and with-doc to true by default, which in my opinion makes little sense, especially in light of #4541 as it does not take into account user behaviour at all but some hypothetical "what if someone had OPAMWITHTEST=1 and OPAMWITHDOC=1 always set in their environment".

This draft PR flips the default for those two variables to use what the actual default everywhere else is (with-test=false and with-doc=false). It also gets rid of -i which has no purpose anymore.

Instead this PR introduces three more arguments: --with-test, --with-doc and --with-dev-setup. However in light of #4541 i'm not sure this is a good idea and i don't this anyone uses opam admin check without -i currently, which makes the introduction of those replacements seem useless. What do you think?

Related to #5805

@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 10, 2024
@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Dec 10, 2024
@kit-ty-kate kit-ty-kate mentioned this pull request Dec 10, 2024
Closed
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Dec 12, 2024

I like this.

@kit-ty-kate kit-ty-kate mentioned this pull request Dec 12, 2024
Closed
rjbou
rjbou reviewed Dec 16, 2024
View reviewed changes
@kit-ty-kate kit-ty-kate force-pushed the admin-check-test-doc-dev-setup branch from 281ec79 to df17b06 Compare December 16, 2024 14:40
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 16, 2024
@kit-ty-kate kit-ty-kate force-pushed the admin-check-test-doc-dev-setup branch from df17b06 to 82450a7 Compare December 16, 2024 14:47
@kit-ty-kate kit-ty-kate marked this pull request as ready for review December 16, 2024 14:47
@kit-ty-kate
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Dec 16, 2024

After reflection i chose to not add new arguments --with-test, --with-doc and --with-dev-setup as they would be highly misleading as they would set the variable globally instead of the intended per package, the former does not make sense and is not possible to have elsewhere in opam anyway.

The PR is now ready to review

@kit-ty-kate kit-ty-kate requested a review from rjbou December 17, 2024 18:51
rjbou
rjbou reviewed Jan 15, 2025
View reviewed changes
master_changes.md
* ◈ Add `opam admin compare-versions` to compare package versions for sanity checks [#6197 @mbarbin]
* [BUG] Fix `opam admin check` in the presence of the `with-dev-setup` variable [#6331 @kit-ty-kate - fix #6329]
* ✘ The `-i`/`--ignore-test-doc` argument has been removed from `opam admin check` [#6335 @kit-ty-kate]
* ✘ `opam admin check` now sets `with-test` and `with-doc` to `false` instead of `true` [#6335 @kit-ty-kate]
Copy link
Copy Markdown
Collaborator

@rjbou rjbou Jan 15, 2025

Choose a reason for hiding this comment

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

Suggested change
* ✘ `opam admin check` now sets `with-test` and `with-doc` to `false` instead of `true` [#6335 @kit-ty-kate]
* ✘ `opam admin check` now sets `with-test`, `with-doc`, and `with-dev-setup` to `false` instead of `true` [#6335 @kit-ty-kate]

Copy link
Copy Markdown
Member Author

@kit-ty-kate kit-ty-kate Jan 18, 2025

Choose a reason for hiding this comment

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

with-dev-setup wasn't set at all before and has been set to false already in #6331

@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Jan 15, 2025

After reflection i chose to not add new arguments --with-test, --with-doc and --with-dev-setup as they would be highly misleading as they would set the variable globally instead of the intended per package, the former does not make sense and is not possible to have elsewhere in opam anyway.

Agree. Plus, as there is no way to request check for a single package there is nothing else to add.

@kit-ty-kate kit-ty-kate force-pushed the admin-check-test-doc-dev-setup branch from 82450a7 to ad452a9 Compare January 18, 2025 13:12
@kit-ty-kate
Copy link
Copy Markdown
Member Author

kit-ty-kate commented Jan 18, 2025

rebased above master

@kit-ty-kate kit-ty-kate merged commit 0a73eab into ocaml:master Jan 20, 2025
@kit-ty-kate kit-ty-kate deleted the admin-check-test-doc-dev-setup branch January 20, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjbou rjbou rjbou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.4.0~alpha1

Development

Successfully merging this pull request may close these issues.

3 participants

@kit-ty-kate @hannesm @rjbou