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

Add linting check for restricted prefixes #316

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

benmandrew
Copy link
Contributor

Fixes #315.

@benmandrew benmandrew requested a review from shonfeder May 27, 2024 16:36
@raphael-proust
Copy link

The list of conflict-class (that we want the linter to help keep in sync with the prefixes) is discussed on ocaml/opam-repository#25861 (review)

The discussion is not complete (as time of writing of this comment).

@benmandrew
Copy link
Contributor Author

The current prefix to conflict class mapping according to my understanding is:

  • arch- to ocaml-architecture-setting
  • system- to ocaml-system-setting
  • host-arch- to ocaml-host-archicture
  • host-system- to ocaml-host-system
  • msys2- to msys2-environment

This can easily be changed as the discussion evolves.

Copy link
Contributor

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Looking good! I have one question and related suggestion about the new-package detection logic. My other suggestions are minor documentation and testing-related things.

lib/lint.ml Show resolved Hide resolved
lib/lint.ml Outdated Show resolved Hide resolved
lib/lint.ml Outdated Show resolved Hide resolved
lib/lint.ml Outdated Show resolved Hide resolved
test/lint.t Outdated Show resolved Hide resolved
lib/lint.ml Outdated Show resolved Hide resolved
@benmandrew benmandrew force-pushed the restricted-prefixes branch from b1627f5 to 450a7e5 Compare June 4, 2024 12:34
@benmandrew
Copy link
Contributor Author

Recommendations have been integrated and the PR has been rebased onto master; now we're waiting for the finalisation of the discussion on ocaml/opam-repository.

@shonfeder
Copy link
Contributor

The list of conflict-class (that we want the linter to help keep in sync with the prefixes) is discussed on ocaml/opam-repository#25861 (review)

The discussion is not complete (as time of writing of this comment).

@raphael-proust could you confirm whether the prefix/conflict classes here match what you are expect from a maintenance angle?

@raphael-proust
Copy link

The prefix (presented as a glob) / conflict classes (presented as a literal string) that were settled on are:

mysys2-* / "msys2-env"
arch-* / "ocaml-arch"
ocaml-env-mingw* / "ocaml-env-mingw"    -- NOTE: the globbing pattern for file names doesn't have a dash
ocaml-env-msvc* / "ocaml-env-msvc"      -- NOTE: same
host-arch-* / "ocaml-host-arch"
host-system-* / "ocaml-host-system"
system-* / "ocaml-system"

@shonfeder shonfeder requested a review from punchagan June 11, 2024 20:30
@shonfeder
Copy link
Contributor

@punchagan since I've touched this a fair bit and you're looking at the linting stuff anyhow, would mind reviewing this when you have a chance?

@punchagan punchagan force-pushed the restricted-prefixes branch from c935b4f to f9e4507 Compare June 20, 2024 13:24
@punchagan
Copy link
Contributor

@punchagan since I've touched this a fair bit and you're looking at the linting stuff anyhow, would mind reviewing this when you have a chance?

This looks good to me. I ended up re-ordering and splitting up the commits slightly to split out the refactor commits and the changes that add the new lint checks.

We could move the "analysis" to see if a package is a newly published one to the analysis module, as discussed yesterday. But, it could also come as a separate change.

@shonfeder shonfeder force-pushed the restricted-prefixes branch from f9e4507 to 740bb89 Compare June 20, 2024 15:17
@shonfeder
Copy link
Contributor

Thank you for the review and the cleanup, @punchagan! I've made a no-op change just to sign the commits so I get the green label XD

We could move the "analysis" to see if a package is a newly published one to the analysis module, as discussed yesterday. But, it could also come as a separate change.

I think that would be the right way, but locating that check in the linting rather the analysis was already inherited in this work. If anything, that should be a followup I think, as it would possibly affect other parts of the pipeline. However, I think we may just postpone this entirely, as we are liable to be replacing all of this with the propotype work soon.

@shonfeder shonfeder merged commit 97d42b7 into ocurrent:master Jun 20, 2024
2 checks passed
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.

Add linting check for new restricted prefixes
4 participants