Drop support for python < 3.5

[a-detiste]

No description provided.

[AdamWill]

Thanks! As you can see, though, needs some fixes. Yay for tests :)

I was planning to do a kinda more extensive cut at this - use more modern features like f-strings - but I never got around to it, so I'm fine with a more minimal version, if you can fix the failures. Thanks again.

[a-detiste]

annotations + mypy is a more usefull modern feature than f-string... I think

[AdamWill]

sure, that would be useful too!

[a-detiste]

that would be useful too!

That would be usefull right now ,
it would have told me I'm trying to stuff unexpected content in self._iter

If the test fails again I'll do that.

If you want it I can do it anyway in an other PR.

[AdamWill]

I am all for type annotations, seems like it'd be quite a big job for this library though?

This LGTM now, let's give it a bit of time for other maintainers to take a look too.

[a-detiste]

Yes, one thing at a time :-)

The framework evolves fast,
so you might after a while decide that 3.5 is too old as a baseline.

It can be iterative, with a pretty good cost/gain ratio.

Here for example I dig up what was a 'site:',
bot not yet 'return_values:'.

--- a/mwclient/listing.py
+++ b/mwclient/listing.py
@@ -2,6 +2,9 @@ from mwclient.util import parse_timestamp
 import mwclient.page
 import mwclient.image
 
+from typing import Optional, TYPE_CHECKING
+if TYPE_CHECKING:
+    from mwclient.client import Site
 
 class List:
     """Base class for lazy iteration over api response content
@@ -11,9 +14,17 @@ class List:
     continuing content.
     """
 
-    def __init__(self, site, list_name, prefix,
-                 limit=None, return_values=None, max_items=None,
-                 *args, **kwargs):
+    def __init__(
+        self,
+        site: 'Site',
+        list_name: str,
+        prefix: str,
+        limit: Optional[int] = None,
+        return_values=None,
+        max_items: Optional[int] = None,
+        *args,
+        **kwargs
+    ) -> None:
         # NOTE: Fix limit
         self.site = site
         self.list_name = list_name

[AdamWill]

Personally I tend to use 3.6 as the baseline as I work for Red Hat and it's in the system Python in some RHEL or other :P But this project is kinda collaborative - the original maintainers have all gone inactive, now a few of us who are active users of the library are trying to take care of it. So we'll have to figure it out together.

[marcfrederick]

This looks great, thank you @a-detiste! Typing support will be great for this library and this will help us get there.

As for the minimum version, I think I'd also drop 3.5 while we're at it to get f-strings and variable annotations.

[NguoiDungKhongDinhDanh]

tox.ini also needs to be modified so as to remove py27 (as well as 34 and 35, if we were to do this thoroughly) from envlist:

[tox]
-envlist = py27,py35,py36,py37,py38,py39,py310,py311,py312,flake
+envlist = py35,py36,py37,py38,py39,py310,py311,py312,flake

[a-detiste]

so this is done ?

[marcfrederick]

Hey @a-detiste thanks again for you're contribution. I found two more small issues in the upload.py example, apart from that this looks good to me and I would be in favor of getting this merged.

[a-detiste]

I pushed more changes, I guess it's better to squash everything in the merge.

[marcfrederick]

Thanks, from my side, this looks good now. The baseline is still 3.5 but, I think we can defer the decision on whether to drop that in favor of 3.6 to another discussion and proceed with getting this one merged.

[AdamWill]

Can we update the commit message on the first commit from 3.3 to 3.5, in case that gets used however we merge this?

[AdamWill]

other than that I guess this looks good.

[a-detiste]

done

[a-detiste]

Ping

[AdamWill]

Sorry, have not had a lot of time for mwclient stuff lately with $DAYJOB :( merging, thanks!

[AdamWill]

ugh, actually - @marcfrederick , I don't remember what was the merge strategy we agreed on. For my own projects I use rebase but I remember you didn't like that. Are we using squash and merge?

[marcfrederick]

No, I also prefer using rebase and tend to use that for my personal projects. I've only used merge here because I saw you using that. Let's just go with rebase merging (and possibly enforce that in the settings?).

[AdamWill]

ugh, jeez, well certainly someone wanted to use a different merge strat. maybe @waldyrious ?

[AdamWill]

Well, it's been two weeks and the only maintainers who replied like rebasing, so let's rebase. :D Thanks again for the work!

[waldyrious]

ugh, jeez, well certainly someone wanted to use a different merge strat. maybe @waldyrious ?

Oh yeah, that was me — sorry it took me so long to reply here. So, for future reference, the strategy in question consists in rebasing a branch (if it's not based on the latest commit of the target branch) and then merging via a merge commit (instead of via squashing or a fast-forward merge).

This method —explained here in more detail, including a helpful diagram— is sometimes named "semi-linear", as it generates a nearly linear git history, while still making use of merge commits that allow multiple commits from a branch to be grouped when navigating the history. (Of course, it's only useful to preserve the separate commits if they are indeed atomic, deliberately separated and well-described.

In any case, that was just my personal preference; it never was encoded in the project's contribution documentation, so unless you like the idea, feel free to continue using the rebase + fast-forward merge method.