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

Add Python 3 support #40

Closed
wants to merge 73 commits into from
Closed

Conversation

PetrDlouhy
Copy link
Contributor

This PR is adding Python 3 support.

Not all tests are working yet, fell free to complete this PR.

Fix for #698

@mgedmin
Copy link
Contributor

mgedmin commented Feb 13, 2017

Do you plan to rebase this (and resolve conflicts)?

@PetrDlouhy
Copy link
Contributor Author

@mgedmin Rebased. I don't have time at the moment for fixing the remaining failing tests though.

@mgedmin
Copy link
Contributor

mgedmin commented Feb 14, 2017

Thank you!

I'll try to take a look at this when I find more free time. Feel free to ping periodically, so I don't forget!

@mgedmin
Copy link
Contributor

mgedmin commented Mar 14, 2017

Oh dear, the diff is +10,500 −3,394, I don't know if I can find that much free time :/

@anarcat
Copy link
Contributor

anarcat commented Mar 14, 2017

well travis should pass first. then remove the "uncomplete" [sic] tag when done...

@anuj-ssharma
Copy link

Do we know when this will support 3.6 ?

@anarcat
Copy link
Contributor

anarcat commented Oct 30, 2017

as long as the build fails, this is stalled and needs someone to rewrite this patch.

@PetrDlouhy PetrDlouhy force-pushed the python3 branch 6 times, most recently from 276340a to 4535a5c Compare January 6, 2018 20:06
@PetrDlouhy
Copy link
Contributor Author

I have rewritten this. Now the Python 2 tests are working (and should be working for all commits).
The Python 3 tests are running in allow-failures mode. There are few failing tests for 3.5, but the in the tests for version 3.6 the HtmlParser segfaults which breaks whole testing.

I will try to fix these tests, but it might be a bit more complicated. So the current state could be OK for merging (after review and some testing).

I tried linkchecker on few pages and it works OK on Python 3, so the failing test could be just edge cases.

Most of the volume of this patch is update of dnspython and miniboa-r42 in the third_party directory. Which I directly copied from their sources. These should probably be taken out from the sources and used as dependencies, but it is out of the scope of this PR.

@PetrDlouhy
Copy link
Contributor Author

I managed to remove the third party libraries in #118. So after pulling that, this PR would be much tidier.

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented Jan 9, 2018

This is mostly finished. It would require to enable run tests for Python 3.5 (at least) to be enabled in normal mode. But there are 3 remaining failing tests. I am not sure, if I can get them fixed, because they are caused by some encoding magic in the c++ parser. Also it could be waste of time, if the Linchecker would use external parser as suggested in #119.

Anyway, this could probably be pulled after some testing and review, because it should not affect running on Python 2 and allow to run it on Python 3 (although it might not work in some cases).

@PetrDlouhy PetrDlouhy changed the title (Uncomplete) Add Python 3 support Add Python 3 support Jan 9, 2018
Copy link
Contributor

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

I would be much happier reviewing and merging 50 smaller incremental Python 3 support requests, than swallowing one large one. Perhaps your existing commits could be cherry-picked into new PRs?

I don't know if that's too much to ask.

requirements.txt Outdated
# optional:
argcomplete
future
six
Copy link
Contributor

Choose a reason for hiding this comment

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

Both future and six? ;)

(I like six. I don't like future, but I think I got it mixed up with pies2overrides in my mind, which is evil and actually caused me pain, when it got pulled in as a side effect of isort a while back).

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, these need to go into install_requires as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, good remark. I used six only on one place and it was absolutely not necessary. So now it is reworked and in install_requires.

@anarcat
Copy link
Contributor

anarcat commented Jan 10, 2018

so there are conflicts here and support @mgedmin's comments: can't we do this one step at a time? and shouldn't we do this after the massive changes behind the parser and the dependency cleanups are in?

@PetrDlouhy PetrDlouhy force-pushed the python3 branch 2 times, most recently from 3ee6710 to d31cbf9 Compare January 12, 2018 10:33
@PetrDlouhy
Copy link
Contributor Author

@anarcat @mgedmin I have rebased this and reworked by your suggestions.
It is possible to split this into several PRs.
I think, that many changes are not colliding with the parser change. So I will try to prepare some PRs with the easiest and most obvious changes for now.

@anarcat
Copy link
Contributor

anarcat commented Jan 12, 2018

great thanks!

@SchoolGuy
Copy link

@mgedmin Can I help somehow? I have some bugs in the opensuse bugzilla which would probably get closed if this PR gets merged. Also I could start building the linkchecker-gui package in the OBS.

@mgedmin
Copy link
Contributor

mgedmin commented Jan 2, 2019

@SchoolGuy a series of small PRs would help best (small == easy to review, e.g. each PR focusing on one kind of change, ideally under 300 lines of diff).

@SchoolGuy
Copy link

@mgedmin So basically splitting this PR is your favourited solution?

@anarcat
Copy link
Contributor

anarcat commented Jan 6, 2019

@mgedmin So basically splitting this PR is your favourited solution?

yes.

@anarcat anarcat mentioned this pull request Feb 20, 2019
@yarikoptic
Copy link
Contributor

FWIW, if reviewing one big PR is too much, there is always a choice of reviewing each commit separately. They seems to be "atomic" and bite-size enough. Otherwise, for such migration as 2to3 making isolated small PRs might be futile since making one (sub)module work, would entail making 20 others it imports one way or another work too.

@yarikoptic
Copy link
Contributor

TODO

  • change .travis.yml to not allow failures with python 3.x

@anarcat
Copy link
Contributor

anarcat commented Mar 25, 2019

just to be clear, the desire to split this up is to make it easier for us to review, and it is based from this comment from @PetrDlouhy:

It is possible to split this into several PRs.
I think, that many changes are not colliding with the parser change. So I will try to prepare some PRs with the easiest and most obvious changes for now.

Also, any GitHub user can review this PR, not just us project admins, so if you think you can help, please do!!

That said, I don't think per-commit reviews is practical on GitHub: I find it cumbersome to browse around the commits in a PR. But maybe that's just me...

The thing with the PR right now is it needs pass CI and, because it hasn't been updated in a while, rebased to fix the merge conflicts... Maybe that can be done on top of the PR already...

@cjmayo
Copy link
Contributor

cjmayo commented Apr 9, 2019

I've had a go at splitting these up:
https://github.com/cjmayo/linkchecker/branches/active

I can create individual PR's for all the python3_nn branches - but wanted to see if there was support for that here before blasting them out. There's just over twenty available now, mostly single commits, they'd need to be committed to master before the next batch could be created.

I've had to rebase this python3 branch and do a few fix-ups, maybe it would be worth creating a not to be merged PR for that to track overall progress and CI test status?

@anarcat
Copy link
Contributor

anarcat commented Apr 9, 2019 via email

@cjmayo cjmayo mentioned this pull request Apr 10, 2019
@cjmayo
Copy link
Contributor

cjmayo commented Apr 10, 2019

OK that's done...
Tracker PR #210 with some explanation, and the first batch is #211 to #231.

@anarcat
Copy link
Contributor

anarcat commented Apr 15, 2019

this is now happening in #210. thanks for spearheading the first effort, @PetrDlouhy - your work was very useful in laying the foundation for the next pass, and I hope you can find time to give us a hand again in the future! :)

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.

None yet

7 participants