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

Tiny refactor of how Cabal handles configure scripts #8648

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

Ericson2314
Copy link
Collaborator

See each commit for details. There are no public interface changes, just shuffling around private details.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 31, 2022

I've restarted doctests CI and now it passes (previously complained about checksums), so probably a random instability.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 31, 2022

As far as I can see, this is indeed a guaranteed safe refactoring, so tests are not mandatory. A changelog file would be welcome, though.

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.

Blocking on a changelog file.

We don't need this function to have this parameter, and it is private so
we don't need to worry about breakage.
@Ericson2314
Copy link
Collaborator Author

I sort of would think this sort of internal-only change would be the type of thing that belongs in the git history but not a change log, but I added a change log entry as requested.

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.

Actually, you are probably right and I was overzealous. This is not even a change of API, because the module is not yet visible. Anyway, it doesn't hurt.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 2, 2023

Please feel free to set a merge label.

@andreasabel andreasabel added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Jan 2, 2023
@Ericson2314 Ericson2314 merged commit eada3ba into haskell:master Jan 3, 2023
@Ericson2314 Ericson2314 deleted the autoconf-refactor branch January 3, 2023 01:53
@ulysses4ever
Copy link
Collaborator

@Ericson2314 please, refer to https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions for a description of how a PR is supposed to be merged.

@andreasabel
Copy link
Member

@Ericson2314 : Ah, yes, me setting the "merge me" label wasn't instructing you to do a merge commit, but this is a command to Mergify to rebase+merge (with a 2 day delay). No great harm done, but for the sake of a linear history rebase+merge (e.g. in your case with clean commits) or squash+merge are preferred.

@Ericson2314
Copy link
Collaborator Author

Ah sorry about that, yes I did indeed misinterpet the label.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 5, 2023
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 type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants