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

ZOOKEEPER-3771: Update zk-merge-pr script to Python3 #1295

Closed
wants to merge 4 commits into from

Conversation

tisonkun
Copy link
Member

@eolivelli generally I use 2to3 util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

@eolivelli
Copy link
Contributor

I will be happy to try out this patch for my next use of the script
thank you for your quick response !

@eolivelli
Copy link
Contributor

can we keep the original file and add a new zk-merge-pr.py3 ?

@tisonkun
Copy link
Member Author

@eolivelli i'd prefer a py2 and use py3 as py default.

@eolivelli
Copy link
Contributor

Ok

@anmolnar
Copy link
Contributor

Great work @tisonkun !
@eolivelli Why do you want to keep the python2 script? Source control does it for you for free.

@tisonkun
Copy link
Member Author

Hi @anmolnar ! Here is part of the original mail

AFAIK python2 is EOL so maybe we can just have only python3 on master
and leave the old version on git repo

I can see one of the reasons we still have a py2 script is that possibly some of our committers still use py2 so that they can easily use the py2 script which keeps previous workflow. Though, a community lazy agreement might help on dropping it.

@eolivelli
Copy link
Contributor

I agree with @tisonkun

@nkalmar
Copy link
Contributor

nkalmar commented Mar 30, 2020

I voted for v3 only on the mail list, but I agree we could keep v2 for a time, but maybe add a warning when script starts that it is deprecated?

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

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

{code}
commit_title = input("Commit title [%s]: " % pr_title.encode("utf-8")).decode("utf-8")
{code}
Doesn't work, no need to decode str in v3 anyomre, it throws an error. Might be other occurrences, please check. Thanks!

edit: removing just the decode only here worked for me, I managed to commit with python3 after this.

@tisonkun
Copy link
Member Author

@nkalmar Thanks for your review and test. That is the only encode/decode pair on str. I pushed a follow-up commit to address it.

@enixon
Copy link

enixon commented Mar 30, 2020

This looks good to me - super happy for py3 support.

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tisonkun !

@asfgit asfgit closed this in a4c97d2 Mar 31, 2020
asfgit pushed a commit that referenced this pull request Mar 31, 2020
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes #1295 from TisonKun/patch-1

(cherry picked from commit a4c97d2)
Signed-off-by: Norbert Kalmar <[email protected]>
@nkalmar
Copy link
Contributor

nkalmar commented Mar 31, 2020

Merged to master and branch-3.6 (branch-3.5 does not have the script in the repo). Thanks @tisonkun !

stickyhipp pushed a commit to stickyhipp/zookeeper that referenced this pull request Aug 19, 2020
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1295 from TisonKun/patch-1
@tisonkun tisonkun deleted the patch-1 branch May 23, 2022 09:17
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1295 from TisonKun/patch-1
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1295 from TisonKun/patch-1
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1295 from TisonKun/patch-1
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
eolivelli generally I use `2to3` util and check the codepath that I can arrive, manually fix some lines.

But it seems we can verify this patch totally when merging this patch :)

Author: tison <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1295 from TisonKun/patch-1
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.

5 participants