Skip to content

Conversation

@raramakr
Copy link
Contributor

The fields are required for handling more use cases
Refactor: Handle directory removal logic(cleaning of build directories) in one function

Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

For future PRs, can you please request reviews from RPM/DEB packaging maintainers (@nunnikri and @frepaul ?) first, before requesting a review from me? I have a very high code review load and these types of change should first be reviewed by someone closer to the given domain.

Comment on lines +89 to +90
# Clean debian build directory
remove_dir(DEBIAN_CONTENTS_DIR)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I'm just noticing that this is a global variable... you could move it into a local variable that gets passed into each function for better encapsulation.

https://google.github.io/styleguide/pyguide.html#25-mutable-global-state

This looks dangerous since it isn't clear where the variable gets set to a valid directory and this is a destructive operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is on our radar and we are making these changes (global to local variable)for the next patch

@raramakr
Copy link
Contributor Author

For future PRs, can you please request reviews from RPM/DEB packaging maintainers (@nunnikri and @frepaul ?) first, before requesting a review from me? I have a very high code review load and these types of change should first be reviewed by someone closer to the given domain.

Sure Scott

…ndling in RPM and Debian packaging

Refactor: Handle directory removal logic(cleaning of build directories) in one function
@raramakr raramakr force-pushed the users/raramakr/native-pkg branch from 1d1c5b8 to 534ed84 Compare November 25, 2025 00:54
@raramakr raramakr merged commit fa8203c into main Nov 25, 2025
7 checks passed
@raramakr raramakr deleted the users/raramakr/native-pkg branch November 25, 2025 00:56
@github-project-automation github-project-automation bot moved this from TODO to Done in TheRock Triage Nov 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants