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

PRs that refactor code #310

Closed
FirefoxMetzger opened this issue Mar 29, 2021 · 2 comments
Closed

PRs that refactor code #310

FirefoxMetzger opened this issue Mar 29, 2021 · 2 comments

Comments

@FirefoxMetzger
Copy link
Contributor

While reading through gym-ignition's source, there are instances where I think code could be re-written to become more readable, e.g. removing commented-out code, or where I think a refactor could combine code paths and lead to less duplication. Generally, PRs which don't affect behavior.

For those cases, should I file a new issue each time, or would you prefer I directly submit a PR that proposes the change (to then be accepted/rejected)? Perhaps some completely different approach?

@diegoferigo
Copy link
Collaborator

Thanks @FirefoxMetzger for asking. We definitely appreciate PRs that improve style, logic, documentation, and tests.

From my side, PRs are the best way to propose minor refactorings. If there isn't any specific bug in master, I'd suggest to always target devel for this kind of contributions so that we can test more the branch before releasing (I personally always use devel on daily basis due to that) [1]. We have tests that should prevent major regressions, however, cosmetic PRs that also extend the test suite are much appreciated, even if this is not mandatory.

[1] The only challenge is when devel and master use a different Ignition distribution, as it will temporarily happen when #307 will be merged into devel. This could create problems to the PR author, depending on thier current setup (i.e. if they cannot use the most recent Ignition version due to local development constraints). In this scenario, we'll try to handle case-by-case.

@FirefoxMetzger
Copy link
Contributor Author

@diegoferigo Answers this question. I'll close the issue. (Maybe it should live over at Discussions?)

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

No branches or pull requests

2 participants