search

rpm-software-management / urlgrabber

GNU Lesser General Public License v2.1
14 stars 23 forks source link

[WIP] Python 3 port of urlgrabber #8

Closed brejoc closed 5 years ago

brejoc commented 5 years ago

With these changes it is possible to use urlgrabber with Python 3, while still be Python 2 compatible.

Please see #7.

brejoc commented 5 years ago

Feedback is very welcome!

Conan-Kudo commented 5 years ago

@brejoc Please don't merge a bunch of independent changes into a single commit like that.

If you want to do unrelated things, please structure like so:

and so on.

We need to be able to understand the changeset after it's merged too, which is why I'm asking this.

brejoc commented 5 years ago

@brejoc Please don't merge a bunch of independent changes into a single commit like that.

…

We need to be able to understand the changeset after it's merged too, which is why I'm asking this.

Yeah, that wasn't very good. Sorry about that!

hroncok commented 5 years ago

@keszybz makes good points about porting... those are all documented at https://portingguide.readthedocs.io/en/latest/process.html#port-the-code - not sure if you are aware of this piece of doc, so sharing it for a reference.

brejoc commented 5 years ago

Thanks, I'll bookmark that link. And yes, and that's roughly what we did here. In the first phase we leveraged 2to3 and in the second phase we tried to iron some things out to make it actually work in Python2 and Python3. The tests where our indicator. While the results might not be the cleanest code on earth, we do have to keep in mind that the starting base also wasn't. The focus was on getting it running and not refactoring it. I'm definitely willing to improve this PR, but one thing should also be clear: I don't have the time and resources to completely redo this. If this would be needed, I'd have to close this PR. My constraints simply wouldn't allow that at the moment.

Right now we are focusing on the integration in the repo sync mechanism of SUSE Manager and some additional issues came up that are partly already fixed or will be fixed in the coming days. After that we can talk about getting this PR into a better shape. I guess chat would be a better tool to do that. @Conan-Kudo, will you be my contact for this endeavor?

Conan-Kudo commented 5 years ago

@brejoc That's fine. What I meant was that the commits should represent a logical working change. I don't really care too much if you make the underlying code cleaner right now. But the idea here is I need to be able to make sense of what you're changing and why.

Does that make sense?

Conan-Kudo commented 5 years ago

This is now done with the 4.0.0 release. Tarballs coming soon!