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

spandsp: refactor #218480

Merged
merged 1 commit into from Mar 10, 2023
Merged

spandsp: refactor #218480

merged 1 commit into from Mar 10, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 26, 2023

Includes (to avoid merge conflict):

Description of changes

The spandsp derivation has the clauses in the wrong order (e.g. makeFlags before configureFlags) which makes it hard to understand.

This commit fixes the clause order, factors out common.nix from the two versions of spandsp, and also tries to normalize the coding style.

CC: @SuperSandro2000 for review

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux

@ghost ghost marked this pull request as draft February 26, 2023 17:46
@alyssais alyssais mentioned this pull request Feb 26, 2023
1 task
@ofborg ofborg bot requested a review from 7c6f434c February 26, 2023 18:15
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 labels Feb 26, 2023
@ghost ghost changed the title spandsp: reformat spandsp: refactor Feb 26, 2023
@ghost ghost marked this pull request as ready for review February 26, 2023 18:18
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 26, 2023
@ofborg ofborg bot requested review from ajs124 and misuzu February 26, 2023 18:59
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Feb 26, 2023
Copy link
Contributor

@misuzu misuzu left a comment

Choose a reason for hiding this comment

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

LGTM! This should probably go to the staging though

@figsoda figsoda added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Mar 2, 2023
@ghost ghost marked this pull request as draft March 7, 2023 09:49
@ghost ghost changed the base branch from master to staging March 7, 2023 09:49
@ghost ghost marked this pull request as ready for review March 7, 2023 09:54
@ghost ghost requested review from misuzu and removed request for ajs124 and 7c6f434c March 7, 2023 09:55
@ofborg ofborg bot requested a review from ajs124 March 7, 2023 11:33
@7c6f434c
Copy link
Member

7c6f434c commented Mar 9, 2023

Wait, it includes the changes, but not the commits? I merged in the wrong order, I guess, but I thought it would work if this PR includes those?

The `spandsp` derivation has the clauses in the wrong order
(e.g. `makeFlags` before `configureFlags`) which makes it hard to
understand.

This commit fixes the clause order, factors out `common.nix` from
the two versions of spandsp, and also tries to normalize the coding
style.
@ghost ghost marked this pull request as draft March 10, 2023 05:47
@ghost ghost marked this pull request as ready for review March 10, 2023 05:47
@ghost
Copy link
Author

ghost commented Mar 10, 2023

Wait, it includes the changes, but not the commits? I merged in the wrong order, I guess, but I thought it would work if this PR includes those?

#218477 got some suggested changes which I forgot to backport into this PR.

It's rebased now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants