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

Merge two Globbing Modules and Fix #5349 #9673

Merged
merged 1 commit into from Feb 17, 2024

Conversation

alt-romes
Copy link
Collaborator

@alt-romes alt-romes commented Jan 30, 2024

Merge the two Globbing modules in cabal and cabal-install

We use the datatype representation from the globbing in cabal-install,
but preserve a standalone parser for globs present in cabal files, whose
specification is constrained by the cabal specification. The
implementations are merged taking the best parts of each.

and

Make sure sdist is robust to accidentally-listed directories

The refactor of the globbing modules re-uncovered this issue, and
required a prompt fix for the refactor not to break some things.

Fixes #5349

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

Cabal/src/Distribution/Simple/Glob.hs Outdated Show resolved Hide resolved
@Kleidukos
Copy link
Member

@ffaf1
Copy link
Collaborator

ffaf1 commented Feb 1, 2024

Also this is Template A.

@alt-romes alt-romes force-pushed the wip/romes/globbing branch 11 times, most recently from 3ac666b to 8c82552 Compare February 5, 2024 12:20
@sheaf sheaf force-pushed the wip/romes/globbing branch 5 times, most recently from c4f092d to 9d30afb Compare February 13, 2024 15:38
@alt-romes
Copy link
Collaborator Author

@andreabedini light ping. Sam has improved the MR and addressed the rest of the review.
Shall we merge it?

fgaz
fgaz previously requested changes Feb 14, 2024
Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

This comment still has to be addressed

Also this is Template A.

@alt-romes
Copy link
Collaborator Author

This comment still has to be addressed

Also this is Template A.

I replied here:

I don't think I have changed any behaviour, except for maybe the directory-not-exists error being reported instead of file-does-not-match, in some situations where the directory does not exist (in others, we would already report this correctly).
I've updated the tests accordingly.

@alt-romes
Copy link
Collaborator Author

@fgaz what specifically do you think we're missing?

@fgaz
Copy link
Member

fgaz commented Feb 14, 2024

Oh, sorry, I missed that comment.

If the Cabal API changed pheraps add a changelog entry and/or @since annotations

@andreabedini
Copy link
Collaborator

I'm ok but please do add a changelog entry and @SInCE annotations and consider explicitly listing the intended exports of Distribution.Simple.Glob.

@sheaf sheaf force-pushed the wip/romes/globbing branch 3 times, most recently from 7cb8fcb to 7b882a3 Compare February 15, 2024 10:00
@sheaf
Copy link
Collaborator

sheaf commented Feb 15, 2024

The only changes in the user-facing API are:

  • addition of the GlobMatchesDirectory constructor of GlobResult,
  • new function globMatches.

I made a separate Distribution.Simple.Glob.Internal module for the internal implementation details, in order to preserve the user-facing API of Distribution.Simple.Glob. Note that Distribution.Simple.Glob.Internal is nonetheless an exposed module of the Cabal library, because cabal-install file monitoring needs access to the internals.

@sheaf
Copy link
Collaborator

sheaf commented Feb 15, 2024

I'm ok but please do add a changelog entry and @SInCE annotations and consider explicitly listing the intended exports of Distribution.Simple.Glob.

Done.

@sheaf sheaf force-pushed the wip/romes/globbing branch 2 times, most recently from 47aa159 to 6475ac4 Compare February 15, 2024 10:08
@sheaf sheaf added the merge me Tell Mergify Bot to merge label Feb 15, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 17, 2024
We use the datatype representation from the globbing in cabal-install,
but preserve a standalone parser for globs present in cabal files, whose
specification is constrained by the cabal specification. The
implementations are merged taking the best parts of each.

We also make sure sdist is robust to accidentally-listed directories,
as the refactor of the globbing modules re-uncovered this issue, and
required a prompt fix for the refactor not to break some things.

Fixes haskell#5349
@mergify mergify bot merged commit fe82d9b into haskell:master Feb 17, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure sdist is robust to accidentally-listed directories
7 participants