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

When generating Paths modules, define functions when used #8220

Merged
merged 1 commit into from
Aug 5, 2022
Merged

When generating Paths modules, define functions when used #8220

merged 1 commit into from
Aug 5, 2022

Conversation

GuillaumeGen
Copy link

@GuillaumeGen GuillaumeGen commented Jun 15, 2022 •
edited
Loading

This addresses #8219

Please include the following checklist in your PR:

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

@fgaz
Copy link
Member

fgaz commented Jun 16, 2022 •
edited
Loading

The module you edited is generated, you should edit the source instead.

Also please restore the pull request template and address the items

@GuillaumeGen
Copy link
Author

GuillaumeGen commented Jun 16, 2022 •
edited
Loading

The module you edited is generated

I saw this mistake when reading the log of the continuous integration, should be fixed now.

I also added the pull request template.

@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch from 8b39c26 to a85093c Compare June 16, 2022 13:10
@GuillaumeGen
Copy link
Author

I edited the commit message to respect the coding conventions.
I do not think that such a small change should go in the Changelog, should it?
Regarding testing, I have no idea of how to write a test for it. If you have some indications on the strategy, it would be very welcome.

@fgaz
Copy link
Member

fgaz commented Jun 16, 2022 •
edited
Loading

I do not think that such a small change should go in the Changelog, should it?

It fixes a bug, so it should have a changelog entry

Regarding testing, I have no idea of how to write a test for it. If you have some indications on the strategy, it would be very welcome.

you could add a test in cabal-testsuite/PackageTests/PathsModule. there is a readme in cabal-testsuite

@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch from a85093c to 82ba87f Compare June 17, 2022 14:41
@GuillaumeGen
Copy link
Author

I added a changelog file. For the test, my question was more "Do you know what could be a relevant test for this modification?" rather "Where should I add my test?".
As I explained in #8219 this issue stroke me when I was running an external program (gazelle_haskell_modules) and I do not know precisely what caused this.

@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch 2 times, most recently from 05a7c45 to f843f7e Compare June 17, 2022 16:56
@GuillaumeGen
Copy link
Author

After further exploration, it happens that this bug happens whenever one tries to generate a Paths_ module with the option --enable-relocatable, hence writing a test was quite straightforward finally.

@Mikolaj
Copy link
Member

Mikolaj commented Jun 17, 2022

There is some obvious keyword to skip a test on Windows that apparently doesn't support relocatable gizmos. Feel free to use that.

@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch 4 times, most recently from bf74351 to c1a98f3 Compare June 20, 2022 11:51
@GuillaumeGen
Copy link
Author

Is there anything expected from me for this pull request to move forward?

@Mikolaj
Copy link
Member

Mikolaj commented Jul 12, 2022

Unless I'm mistaken, a test was failing previously? If you don't mind I'd rebase and see how it goes now.

@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch from c1a98f3 to 08127b6 Compare July 12, 2022 13:23
@GuillaumeGen
Copy link
Author

Yes, a test was failing the last time you commented it. I followed your advice and used skipIfWindows.
I just rebased.

Mikolaj
Mikolaj approved these changes Jul 12, 2022
View reviewed changes
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.

Congrats, it seems to test fine now. Great job, We need a second review to merge.

@Mikolaj Mikolaj requested review from jneira and fgaz July 12, 2022 15:36
@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Jul 12, 2022
@ulysses4ever
Copy link
Collaborator

I guess, @jneira has had a lot of stuff on his plate lately, and haven't got to this yet. And I'm afraid I'm completely out of my depth here, so I don't feel confident approving (I'd love to!).

I have a question, though: is this related to the --relocatable specifically? If yes, could you by any chance look into improving the docs for it while you're on it? The current doc is terrible imo, and anything would be an improvement.

@jneira jneira linked an issue Aug 1, 2022 that may be closed by this pull request
Inconsistent Paths_ modules generated #8219
Closed
jneira
jneira approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Thanks for the ping, i totally missed this notification, sorry for the dealy.

The pr code looks good and includes the correlated changelog and tests so lgtm, many thanks for fixing the bug.

I would merge it as is as the pr had to wait more than needed.

@jneira
Copy link
Member

jneira commented Aug 1, 2022 •
edited
Loading

I have a question, though: is this related to the --relocatable specifically? If yes, could you by any chance look into improving the docs for it while you're on it? The current doc is terrible imo, and anything would be an improvement.

It seems the author hit the issue trying some code and the connection with relocatable was later (it helped to find out a test case).
Although docs are screaming to be completed i would do it in another pr.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@tweag needs to authorize modification on its head branch.
err-code: 57C92

@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

@GuillaumeGen, @tweag (?), could you kindly rebase and/or permit maintainers to update this branch? Thank you!

@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

refresh

✅ Pull request refreshed

@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

Oh, yes, now I see mergify couldn't merge it due to the missing permissions.

@GuillaumeGen
All the functions used in Paths modules are defined
6c79621 
The functions `splitFileNAme` and `minusFileName` are now defined in the
same conditional block, ensuring that they cannot be used without being
defined.
This fix a bug occurring when generating a Paths_ module with
--enable-relocatable.
@GuillaumeGen GuillaumeGen force-pushed the gg/unconditional-minusFile-func branch from 08127b6 to 6c79621 Compare August 5, 2022 08:03
@GuillaumeGen
Copy link
Author

Thanks a lot for making this go forward!
I just rebased. Is it sufficient?

@GuillaumeGen
Copy link
Author

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 5, 2022

refresh

✅ Pull request refreshed

@Mikolaj
Copy link
Member

Mikolaj commented Aug 5, 2022

I think it's not enough to enable mergify to take care of the PR, but enough for me to now merge manually. Thank you again for the PR!

@Mikolaj Mikolaj merged commit b2c3de3 into haskell:master Aug 5, 2022
@GuillaumeGen
Copy link
Author

Thanks a lot!

@avdv avdv mentioned this pull request Nov 4, 2022
Merged
@avdv avdv mentioned this pull request Nov 21, 2022
Open
@ylecornec ylecornec mentioned this pull request Feb 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikolaj Mikolaj Mikolaj approved these changes

@jneira jneira jneira approved these changes

@fgaz fgaz Awaiting requested review from fgaz

Assignees
No one assigned
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent Paths_ modules generated
5 participants
@GuillaumeGen @fgaz @Mikolaj @ulysses4ever @jneira