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

Update setup.py #3159

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update setup.py #3159

wants to merge 6 commits into from

Conversation

r0cketdyne
Copy link

Removed unnecessary comments and whitespace to make the code cleaner and more concise.

Simplified the check_env_flag function for better readability.

Consolidated the definition of PACKAGE_VERSION to make it cleaner.

Removed redundant environment variable checks and simplified their usage.

Cleaned up the CustomBuild and CMakeBuild classes, removing redundant code and improving readability.

Simplified the CMakeExtension class.

Reorganized the imports for better readability.

Removed unnecessary conversions and simplified some code blocks.

Simplified the setup() function call for better readability.

Removed unnecessary comments and whitespace to make the code cleaner and more concise.

Simplified the check_env_flag function for better readability.

Consolidated the definition of PACKAGE_VERSION to make it cleaner.

Removed redundant environment variable checks and simplified their usage.

Cleaned up the CustomBuild and CMakeBuild classes, removing redundant code and improving readability.

Simplified the CMakeExtension class.

Reorganized the imports for better readability.

Removed unnecessary conversions and simplified some code blocks.

Simplified the setup() function call for better readability.
@stellaraccident stellaraccident self-requested a review April 13, 2024 13:06
@vivekkhandelwal1
Copy link
Collaborator

Hi @r0cketdyne, please address the comments. The PR has been pending there for quite some time.

licence was preserved after shebang line. env var usage docs have been retained as requested by stellaraccident.
@r0cketdyne
Copy link
Author

Hi @r0cketdyne, please address the comments. The PR has been pending there for quite some time.

PR has been resolved. license preserved and relevant docs at env var usage retained.

@vivekkhandelwal1
Copy link
Collaborator

Hi @r0cketdyne, please address the comments. The PR has been pending there for quite some time.

PR has been resolved. license preserved and relevant docs at env var usage retained.

There's still a conflict in this PR?

@r0cketdyne
Copy link
Author

Hi @r0cketdyne, please address the comments. The PR has been pending there for quite some time.

PR has been resolved. license preserved and relevant docs at env var usage retained.

There's still a conflict in this PR?

resolved.

@r0cketdyne
Copy link
Author

2 workflows awaiting approval @stellaraccident

@r0cketdyne
Copy link
Author

iterating on this now.

@vivekkhandelwal1
Copy link
Collaborator

Hi @r0cketdyne, can you please do the formatting changes?

@r0cketdyne
Copy link
Author

Hi @r0cketdyne, can you please do the formatting changes?

Yes, will proceed if not today, tomorrow.

formatting changes.
Copy link
Author

@r0cketdyne r0cketdyne left a comment

Choose a reason for hiding this comment

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

formatting changes.

@r0cketdyne
Copy link
Author

@stellaraccident

@r0cketdyne
Copy link
Author

@vivekkhandelwal1

fixed EOF by adding newline
Copy link
Author

@r0cketdyne r0cketdyne left a comment

Choose a reason for hiding this comment

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

fixed EOF

@r0cketdyne
Copy link
Author

@vivekkhandelwal1 fixed EOF.

Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

@r0cketdyne I see a lot of comments from the setup file being removed in this PR. What's the reason for that?

@r0cketdyne
Copy link
Author

@r0cketdyne I see a lot of comments from the setup file being removed in this PR. What's the reason for that?

I simply forgot to put them back. at arch I had LDXE open and compared diffs and filtered some comments out to make it easier to spot the mismatch. Everything, as it sits should be functional.

Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

Thank you for the patch. Generally, formatting/cleanup patches should be labeled "[NFC]" (non functional change) with a simple description, and in general we separate those from commingled functional changes. At first, it looked like there were no functional changes, but the imports and console scripts are materially changed.

In terms of the formatting changes, cleaning things is nice, but I'm not sure that I would introduce the churn for something like this. If you'd like to proceed, please post a patch that only contains NFC/formatting changes with a very concise description.

For the setup args changes, that needs its own patch with an explanation of why the changes are being made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants