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

Python 3 changes plus automated testing #5

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Conversation

cclauss
Copy link
Collaborator

@cclauss cclauss commented Jun 14, 2019

This seems to be blocking Python 3 progress at nodejs/node#29451 (comment)

Test results: https://travis-ci.com/cclauss/tap2junit

@jbergstroem Your review please. Will require a new release to PyPI #8

@sam-github @refack @bnoordhuis @Trott Your reviews too please.

@@ -10,4 +10,3 @@ junit-xml = "*"
[dev-packages]

[requires]
python_version = "2.7"
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be "2.7,3.7", to reflect what's being used in the Travis config?

Copy link
Collaborator Author

@cclauss cclauss Sep 21, 2019

Choose a reason for hiding this comment

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

I believe that leaving it out is in alignment with the docs... https://pipenv-fork.readthedocs.io/en/latest/basics.html#specifying-versions-of-python

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sure :)

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Lgtm.

tap2junit/__main__.py Outdated Show resolved Hide resolved
@sam-github
Copy link

As long as this continues to work on py2 and py3, it LGTM.

Who has credentials to land it?

@cclauss once landed, do you have pypi publish credentials now?

@cclauss
Copy link
Collaborator Author

cclauss commented Sep 23, 2019

Only @jbergstroem can land it and only @jbergstroem has the PyPI credentials.

@MattIPv4
Copy link
Member

MattIPv4 commented Sep 23, 2019

Alternative plan if this isn't landed soon, as this is a blocker?
Vendor in a fork of this with our fixes where it is needed?

@cclauss
Copy link
Collaborator Author

cclauss commented Sep 23, 2019

See: nodejs/build#1921 for all the places in ansible scripts where it is installed. I kinda hate vendoring in.

@MattIPv4
Copy link
Member

MattIPv4 commented Sep 23, 2019

We could also publish the fork on PyPi under tap2junit-node or similar?

I realise neither are great solutions, but we should have a plan if this is a blocker for moving to Python 3.

@sam-github
Copy link

sam-github commented Sep 23, 2019

@jbergstroem In the interests of minimizing maintenance burden for you, if you are willing, you could just add either @cclauss , or me (@sam-github), or any other Node.js TSC member to admin on this repo, I think that would allow us to do the maintenance.

But if you have time to add cclauss to pypi and transfer it yourself, that works great, too!

I'm not a fan of ansible, and would be happy to vendor it, but I'm not keen to grout out all the references to it in our ansible scripts. The low-maintenance fix is definitely to transfer it, if Johan is willing.

@sam-github
Copy link

@cclauss I think you can land this, can you try?

PR-URL: nodejs#5
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matt (IPv4) Cowley <me@mattcowley.co.uk>
@cclauss cclauss merged commit ee5c20b into nodejs:master Sep 24, 2019
@cclauss cclauss deleted the patch-1 branch September 24, 2019 21:02
sam-github pushed a commit that referenced this pull request Oct 4, 2019
PR-URL: #5
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matt (IPv4) Cowley <me@mattcowley.co.uk>
sam-github pushed a commit that referenced this pull request Oct 4, 2019
Floating patch for Python 3 support.

PR-URL: #5
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matt (IPv4) Cowley <me@mattcowley.co.uk>
sam-github pushed a commit that referenced this pull request Oct 4, 2019
Floating patch for Python 3 support.

PR-URL: #5
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matt (IPv4) Cowley <me@mattcowley.co.uk>
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.

4 participants