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.

Sign up for GitHub

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

Jump to bottom

Add lint test #4967

Merged
merged 2 commits into from
Feb 28, 2022
Merged

Add lint test #4967

merged 2 commits into from
Feb 28, 2022

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Dec 15, 2021 •
edited

related # 4960
/cc @kit-ty-kate
fix # 4989
Only add test, other PR fix linting issues

@rjbou rjbou added KIND: BUG PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Dec 15, 2021
@rjbou rjbou added this to the 2.2.0~alpha milestone Dec 15, 2021
@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Dec 15, 2021
rjbou
rjbou commented Dec 15, 2021
View reviewed changes
Copy link
Collaborator Author

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Some lint cases are doubled (?) or hidden by a file format error (?)

tests/reftests/lint.test Outdated Show resolved Hide resolved
tests/reftests/lint.test Outdated Show resolved Hide resolved
tests/reftests/lint.test Outdated Show resolved Hide resolved
tests/reftests/lint.test Outdated Show resolved Hide resolved
tests/reftests/lint.test Outdated Show resolved Hide resolved
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 19, 2021
@rjbou
Copy link
Collaborator Author

rjbou commented Jan 4, 2022

Some lint errors are triggered first by a file format error (E3). We can:

  • disable those specific lint errors and keep only the format ones (the reason is specified in the message)
  • change OpamFile.OPAM.format_errors type to contain error reason, to retrieve it on its specific lint error

@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Jan 4, 2022
@rjbou rjbou changed the title Add lint test + fix linting a repository package Add lint test + fixes Jan 13, 2022
@rjbou rjbou added the PR: WIP Not for merge at this stage label Jan 13, 2022
@rjbou rjbou moved this from PR to review to PR in progress in Opam 2.2.0 Jan 14, 2022
@rjbou
Copy link
Collaborator Author

rjbou commented Feb 9, 2022

My mistake, I always forget that reftests uses OPAMSTRICT... No file format error that hide lint in normal case

@rjbou
Copy link
Collaborator Author

rjbou commented Feb 9, 2022

Some errors remain unreachable, but because parsing clean them beforhand (E43 & E28, see test)

@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Feb 9, 2022
@rjbou rjbou removed the PR: WIP Not for merge at this stage label Feb 9, 2022
@rjbou rjbou changed the title Add lint test + fixes Add lint test Feb 25, 2022
@rjbou rjbou moved this from PR to review to PR finalised (merge with CI) in Opam 2.2.0 Feb 25, 2022
@rjbou rjbou merged commit 26534d1 into ocaml:master Feb 28, 2022
Opam 2.2.0 automation moved this from PR finalised (merge with CI) to Done Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
KIND: BUG
Projects
Opam 2.2.0
  
Done
Milestone
2.2.0~alpha
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@rjbou