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

cabal-install: check: Whitelist doc file extensions #8747

Merged
merged 3 commits into from
Feb 12, 2023

Conversation

wismill
Copy link
Collaborator

@wismill wismill commented Feb 9, 2023

Fixes #8746

Please also shortly describe how you tested your change. Bonus points for added tests!

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

Copy link
Member

@andreasabel andreasabel 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 extensions should include .TXT and maybe more generally cater for case-insensitive file systems.

isDesirableExtraDocFile paths path = map toLower basename `elem` paths
isDesirableExtraDocFile :: ([FilePath], [FilePath]) -> FilePath -> Bool
isDesirableExtraDocFile (basenames, extensions) path =
map toLower basename `elem` basenames && ext `elem` extensions
Copy link
Member

Choose a reason for hiding this comment

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

ext is not subjected to map toLower here. I wonder how this plays out in case-insensitive filesystems (e.g. on Windows).
It seems that some folks call the file "CHANGELOG.TXT", e.g. https://github.com/moodle/moodle/blob/0780e87f06eb834786f562316ad9699480bcd24e/lib/tcpdf/CHANGELOG.TXT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed and added test.

It is worth to update hackage-server as well (see haskell/hackage-server#1179).

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks!

@andreasabel andreasabel added the squash+merge me Tell Mergify Bot to squash-merge label Feb 9, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 9, 2023

One more thing in the test seems to still require an update. Also please rebase to take advantage of the newest CI fix.

@wismill
Copy link
Collaborator Author

wismill commented Feb 9, 2023

Rebased and (hopefully) fixed.

File order does not seem to be deterministic or is platform-dependent. As we have two similar test cases, I fixed it by keeping only one file in each case.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 12, 2023
@mergify mergify bot merged commit bae536f into haskell:master Feb 12, 2023
@ffaf1 ffaf1 mentioned this pull request Feb 13, 2023
4 tasks
@Mikolaj
Copy link
Member

Mikolaj commented Feb 13, 2023

Am I correct this needs a backport to 3.10?

@wismill
Copy link
Collaborator Author

wismill commented Feb 13, 2023

Yes only if you backported #8657.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 13, 2023

I did not. But it was merged before 3.10 has been cut. Wasn't it?

@Mikolaj
Copy link
Member

Mikolaj commented Feb 13, 2023

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2023

backport 3.10

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 13, 2023
* check: Test only doc files with supported extensions

* Cleanup

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit bae536f)
mergify bot added a commit that referenced this pull request Feb 14, 2023
cabal-install: check: Whitelist doc file extensions (backport #8747)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: cmd/check merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal check wants me to add CHANGELOG.md~
6 participants