salt
salt copied to clipboard
Fix #62281
What does this PR do?
added if-else block to check the python version and fill repo_path with correct data regarding the python major version
What issues does this PR fix or reference?
Fixes: #62281
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:
- Community Wiki
- Salt’s Contributor Guide
- Join our Community Slack
- IRC on LiberaChat
- Salt Project YouTube channel
- Salt Project Twitch channel
There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!
Second commit's code has better performance than if-else's code block repo_path = os.getcwd() if sys.version_info.major == 2: repo_path = os.getcwdu()
this is going to need a testcase and changelog.
Thank you for your comment I will add following changes(test case and changelog)
@whytewolf Could you please take a look at this PR? I would be happy to make any changes needed to this PR since it's been open for a while
did you see the comment about python2 not being supported? I would be happier if we were not importing python2 code.
@whytewolf
did you see the comment about python2 not being supported? I would be happier if we were not importing python2 code.
Sorry, I haven't seen that comment, I removed Python-2 code and sent another commit to remove it Could you check it now?
looks like there is a precommit issue. fix that and I'll approve again.
@whytewolf Hi again Sorry for taking your time too much, it's my first time contributing to a big opensource project I think this time I got it right
looks like the precommit is still failing.
you might need to install it locally and see if you can get it to pass.
What I normally do is
git add .
precommit
git add .
precommit
the second one should pass unless there is something that the precommit doesn't fix automatically.
@whytewolf Thank you for guiding me I got it right this time by setting up local environment Please check this Thanks
@whytewolf Thank you for guiding me I got it right this time by setting up local environment Please check this Thanks
yeap. approved. pending tests now.
@whytewolf That one test I think it needs to re run?
@whytewolf Everything is done and checks passed
@whytewolf Just wanted to check whether everything is okay
@whytewolf Just wanted to check whether everything is okay
yeap things are looking good. I"ll get this in front of the team today to get merged.
Congratulations on your first PR being merged! :tada: