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

Skip deleted packages in analysis #402

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

punchagan
Copy link
Contributor

Potential fix for comment here.

@punchagan punchagan requested a review from shonfeder December 16, 2024 07:51
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.

I think this fix makes good sense! Thank you for helping ensure the archiving effort can proceed smoothly.

Do you think you'd be able to a test for this cas without too much trouble?

However, outside of the archiving process, I think we do not ever want packages deleted, so I wonder if we should make an issue to add some logic to the linting that would raise a warning for deleted packages? WDYT?

lib/analyse.ml Outdated Show resolved Hide resolved
Currently, the analysis phase doesn't expect any package deletions and
fails with the error "impossible". But, with the new opam-repository
archival process, some packages are expected to be deleted from the main
opam-repository and moved into the archive repository.
@punchagan
Copy link
Contributor Author

However, outside of the archiving process, I think we do not ever want packages deleted, so I wonder if we should make an issue to add some logic to the linting that would raise a warning for deleted packages? WDYT?

Yes, that makes sense. We could also try to detect the archiving process using PR author GitHub handles and restrict deletions only to the opam-repository maintainers. But, even a more light-weight check would be useful.

@punchagan punchagan force-pushed the skip-deleted-analysis branch from 64494c8 to 9ff251b Compare December 17, 2024 07:13
Previously, the analysis phase didn't expect any package deletions. So,
we'd fail with the error "impossible". But, with the new opam-repository
archival process, we allow for deletions, even though only in the
special case of a package being archived.
@punchagan punchagan force-pushed the skip-deleted-analysis branch from 9ff251b to 906d7d7 Compare December 17, 2024 07:15
@punchagan
Copy link
Contributor Author

Do you think you'd be able to a test for this cas without too much trouble?

I've added a cram test for this case.

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.

2 participants