librespot-org / librespot

Open Source Spotify client library
MIT License
4.52k stars 544 forks source link

Check formatting and linting first on workflow #1150

Closed yubiuser closed 1 year ago

yubiuser commented 1 year ago

Running the test workflow is a time and resource consuming task - the workflow runs between 20 and 40 minutes. Currently, the heavy test (compiling and cross-compiling) run before the lighter linting and format checking. This follows the following reasoning:

The layering here is as follows, checking in priority from highest to lowest:

However, I think the order should be changed to check formatting and linting first:

  1. It's very annoying to wait 40 minutes for the checks to see it failing during the last steps due to a minor formatting error.
  2. Reduce computational costs (energy): check formatting takes only seconds, compiling up to 30 minutes. If there is any failure (formatting or compiling) we should fail fast to prevent unnecessary computation. Therefore the "faster" checks should be done first.
roderickvd commented 1 year ago

Before I was the one who made this so for the same reasons but vice versa:

  1. Most important in the workflow is working code
  2. Code style comes second
  3. It was irritating to judge PRs by failing CIs with perfectly fine code.

But I actually like your point about energy consumption.

What do others think?

eladyn commented 1 year ago

Another option could be to run the style check first, but add a continue-on-error: true for this part. This way, PRs still get checked thoroughly if styling is wrong, but formatting feedback is given as fast as possible.

However, this would defeat the benefit of less energy consumption.

kingosticks commented 1 year ago

Personally I think people should get into the habit of running the linter(s) locally. That would give the fastest feedback and prevent the uninteresting failures propagating here for all to see.

yubiuser commented 1 year ago

Sure, always running linters locally would be the best solution. But until this is done by everyone you might still want to check during the workflow. I don't think running locally and running during the workflow are mutually exclusive.

kingosticks commented 1 year ago

Correct, not mutually exclusive, I didn't mean to imply that.

roderickvd commented 1 year ago

So what do we do? :smile:

yubiuser commented 1 year ago

It's the burden of the maintainer to decide. But I won't be resentful if the PR is rejected ;-)

roderickvd commented 1 year ago

So I'll go ahead and merge this one, we can always change later if we want to.