Improve tooling and add formatter

[Lotram]

Hello,

While having a closer look at the code before trying to start a MR for #998, I noticed there is no formatter in this repo. I then considered adding one (and maybe a type checker ?), I then saw you @ryneeverett rejected this MR #1114 .

I can get why you refused it, since there is no description, no comment, and it looks like it was mostly done by an AI without human review, still I think using ruff could be beneficial here, at least to format the code. black + isort is also an option, though I think ruff makes more sense in 2025.

I'm open to do it myself, but I wanted to check with you first, as a maintainer, whether you're open to such a change, and if so, how to do it to ease the review.

I'm suggesting the following changes, they can of course be done separately:

• switch to ruff as both a formatter and a linter
• use a type-checker (either mypy, pyright, or astral's ty)
• add pre-commits for these tools, as there is already a pre-commit config.

[ryneeverett]

Thanks for opening this! Your assessment of my rejection of that other pull request is correct.

I looked into ruff and I think it makes a lot of sense. I hope you don't mind that I'm implementing it myself (#1152). I wanted to play around with it a bit before giving you the green light and by the time I was done there wasn't much left to do.

As for type checking, I'm for it. I hadn't seen astral ty but it seems like probably the best choice. I'm currently using pywright in my editor to avoid introducing more issues. But really, if we can get any tool "passing" then we can start running it in tests and avoid future regressions which would be great. My only real concern here is that I hope we don't have to add too much complex boilerplate that makes the code less readable. I haven't played much with typing since the early mypy days though, so I'm assuming the situation has gotten better in this regard.

I'm less convinced of the value of pre-commit but I'm not entirely against it. Rust was really my first positive exposure to formatters and looking at our rust projects taskchampion and taskchampion-sync-server, they do make use of rustfmt but not precommit and it rarely seems to be a problem. Maybe I'm missing something here though...

I'd be thrilled if somebody took on #998 and I can see why you'd want to improve the tooling before taking it on.

[Lotram]

The main advantage of a pre-commit is to reduce the number of failing CI jobs (related to formatting / linting), e.g. if you misconfigured your IDE. So the sooner the failure, the less time you spend waiting for jobs to finish and fail
IMO The only costs of pre-commits are:

1. maintaining another config file, ensuring it's on par with pyproject.toml
2. a delay when committing, which is IMO negligible for those tools, and this codebase

I added two comments on your MR, regarding config.

I haven't used ty yet, I'll try to set it up. I've fixed feeling about typing in python, it can indeed be a pain to configure, with a lot of boilerplate, but considering the size of this project, and as it does not rely too much on other complex libs such as an ORM, I think it should be ok

[ryneeverett]

The only costs of pre-commits are

3. If the user doesn't have their development environment set up right they could be unable to commit and working around precommit is a pain. They might not even be changing python code if they're just tweaking the docs. I suppose the precommit could be set up to succeed if ruff isn't available like ruff format || echo "Ruff formatting failed.".

[ryneeverett]

@djmitche Any thoughts on the use of pre-commit for formatting? Should we be using pre-commit for formatting in our rust projects?

[Lotram]

working around precommit is a pain.

To bypass pre-commits, you can simply use the --no-verify option of git commit ( or -n ).
You can also use SKIP=ruff git commit to bypass a specific pre-commit

[djmitche]

The reason we can get away without pre-commit for Rust is that there's only one way to format Rust code (rustfmt) and pretty much any editor automatically applies this. The result is that it's unusual for someone commit and push un-formatted code.

I've been out of the Python world for a while, but given the choice between Ruff and Black, I assume There's More Than One Way To Do It. So, setting up pre-commit is doing your contributors a bit of a favor by either automatically formatting their code, or alerting them that there are formatting expectations if they don't have the prerequisites installed. Pre-commit is pretty good at installing its prerequisites, though!

[ryneeverett]

Fair enough. I wont implement it myself but I'd accept a PR adding pre-commit.

I've had a couple bad experiences of pre-commit scripts failing in nodejs projects, but I trust that those are anomalous...or just symptoms of the general brokenness of the nodejs world.

[Lotram]

I've been out of the Python world for a while, but given the choice between Ruff and Black, I assume There's More Than One Way To Do It.

Well, I've been very much in the python world for the past 10 years and actually, nowadays, not really : for the past few years, ruff has mostly replaced previous tools in all recent projects, as it's a drop-in replacement for black + flake8 + isort, and much faster.
Astral's ambition is to offer something comparable the Rust tools. And they've succeeded in becoming the de facto standard for both their package manager (uv) and their formatter / linter (ruff).

@ryneeverett I'll set up the pre-commits once your ruff PR has been merged.

[ryneeverett]

Along the lines of improving tooling, I noticed that our codecov CI jobs haven't been working for some time. i don't think we ever really got coverage testing CI configured well enough to be very useful and I'm tempted to just delete what we have. Any opinions or recommendations on this?

[Lotram]

Honestly, I agree, it could be dropped. In my opinion, coverage is necessary on bigger codebases, with more concurrent contributions. If we want to ensure all services are tested, we could simply add a test that ensures we have 1 test file per service, would this be enough ?

Also, as a side-note related to discussion in #1160 there is a pytest-cov, if you want to keep some kind of coverage, while being closer to the test runner (but this would increase pytest dependency).

[Lotram]

Regarding typing, what are your opinion on remove the :param, :rtype and :return parts of the docstring :
Random example

    def _get_repo(self, repo_id: int) -> dict:
        """Queries information about a single repository as JSON dictionary

        :param repo_id: Project ID in the Gitlab instance
        :type repo_id: int
        :rtype: dict

this does not bring any real value, and becomes redundant with typing. Main problem with this kind of docstrings is it can become outdated quite easily. Another example

def find_taskwarrior_uuid(tw, keys, issue):
    """For a given issue issue, find its local taskwarrior UUID.

    Assembles a list of task IDs existing in taskwarrior
    matching the supplied issue (`issue`) on the combination of any
    set of supplied unique identifiers (`keys`).

    :params:
    * `tw`: An instance of `taskw.TaskWarriorShellout`
    * `keys`: A list of lists of keys to use for uniquely identifying
      an issue.  To clarify the "list of lists" behavior, assume that
      there are two services, one having a single primary key field
      -- 'serviceAid' -- and another having a pair of fields composing
      its primary key -- 'serviceBproject' and 'serviceBnumber' --, the
      incoming data for this field would be::

        [
            ['serviceAid'],
            ['serviceBproject', 'serviceBnumber'],
        ]

    * `issue`: An instance of a subclass of `bugwarrior.services.Issue`.

    :returns:
    * A single string UUID.

    :raises:
    * `bugwarrior.db.MultipleMatches`: if multiple matches were found.
    * `bugwarrior.db.NotFound`: if an issue was not found.

Here, both keys and issue have a different type than the one in the docstring.
I'm not saying we should remove the whole comment about keys here, because it gives an example, that's valuable, but I don't think it should give the type (once there is some typing).

I think we could also remove the :params: itself

[ryneeverett]

Yeah, I'm all for removing those kinds of typing comments from docstrings. They were around when I showed up (before we even supported python-3, let alone type hints) and I've never known why. They appear to be sphinx directives but I'm not aware that they were ever used in the docs. I always assumed that some IDE tooling made use of them, but now that python supports type hints they seem thoroughly antiquated.

And yes, I'm also fine with removing any sphinx directives that we're not actually using in the API docs.