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

Have projects import trimmed URIs #10629

Merged
merged 3 commits into from
Dec 25, 2024

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Dec 9, 2024

Fixes #10622. Trims URIs before parsing them with parseURI. If any project config path doesn't equal its trimmed self then it is displayed quoted so that the whitespace is obvious.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

@ulysses4ever
Copy link
Collaborator

Will need a changelog but otherwise it looks like the right direction overall.

@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch 4 times, most recently from 4ebca6f to e27511e Compare December 9, 2024 17:47
@geekosaur
Copy link
Collaborator

Huh. First time I've seen RTD fail that way.

@philderbeast
Copy link
Collaborator Author

Huh. First time I've seen RTD fail that way.

And I didn't change the docs. @geekosaur, other CI jobs I can restart but not that one.

@geekosaur
Copy link
Collaborator

That's normal, since RTD isn't actually part of GitHub Actions. You'll need to change something somewhere to trigger a new build, though. (You may want to hold off a bit though, to let RTD fix whatever's wrong on their end.)

@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch 2 times, most recently from 50b9a2e to f719178 Compare December 12, 2024 19:03
@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch from f719178 to 52978ba Compare December 13, 2024 19:25
@philderbeast
Copy link
Collaborator Author

@geekosaur I've included a W3C quote about URLs in the changelog and added the bug label to this pull request as it fixes a bug.

@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch 3 times, most recently from 3e644b1 to 6c087fb Compare December 15, 2024 11:44
@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch from 6c087fb to fc70bfd Compare December 22, 2024 19:48
@philderbeast
Copy link
Collaborator Author

@Mikolaj this is a bug fix in need of a second reviewer. Do you think we can get it into the upcoming release?

@geekosaur
Copy link
Collaborator

geekosaur commented Dec 22, 2024

Uh, what upcoming release? I'm not sure it qualifies for 3.14.1.1.

@philderbeast
Copy link
Collaborator Author

@geekosaur I had a hunch there was a release coming up based on bumped version numbers and pull requests being expedited. Is there somewhere where plans for upcoming releases are posted?

@geekosaur
Copy link
Collaborator

geekosaur commented Dec 22, 2024

The recently bumped version number was actually missed for 3.14.1.0, and is currently being considered severe enough that we'll push a 3.14.1.1 quickly. But as I understand it, we aren't planning to include any other fixes, just this mistake.

The expedited pull requests are all for the cabal-head prerelease from CI, which has been broken since August except for a single update in October. Its only relationship to releases is to make new features in HEAD available before the next release. Oh, and also for development versions to have commit information in them, which again has nothing useful to do with releases (it's specifically disabled for them).

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

@Kleidukos is the 3.14.* release manager so up them if/when to include this fix.

@Kleidukos
Copy link
Member

Why not include it for a cabal 3.14.2.0, but not for 3.14.1.1 :)

@philderbeast philderbeast force-pushed the fix/project-untrimmed-url branch from ed1e8c9 to 78e1e51 Compare December 23, 2024 12:55
- Trim before isURI check for canonicalizeConfigPath
- Show path quoted if not already trimmed
- Trim before checking with parseURI
- Error if an untrimmed URI is detected
- Add a changelog
- Add UntrimmedImport tests
- Soften the error down to a warning
- Use with-ghc.config trick
- Add fix-whitespace exceptions for test projects
- Include W3C quote is changelog
- Remove unused LANGUAGE pragma
- Rerun test now that URL imports are sorted last
- Move test underneath ProjectImport parent dir
@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Dec 23, 2024
@mergify mergify bot merged commit 731f699 into haskell:master Dec 25, 2024
51 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+no rebase ready and waiting Mergify is waiting out the cooldown period type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cabal treats url with trailing whitespace in import cabal.project file as a file path
5 participants