⚙ D20675 ruff: move rules from .arclint to .ruff.toml

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Schrodinbug: works until I stare at it.

────────────────────────────────────────────────────────────────────────

minor [Maintainability & Code Quality]
→ ]8;;vscode://file//work/.arclint:22.arclint:22]8;;

Keep electrum/electrum-abc in both Arcanist include patterns.

.ruff.toml identifies this extensionless file as a Ruff target. These
patterns now select only .py files, so Arcanist never invokes either
Ruff linter for that script. Add its explicit path to both include
patterns.






Also applies to: 27-27

────────────────────────────────────────
Review complete
1 finding ✔

Minor 1

5 files reviewed:

.arclint
.ruff.toml
arcanist/linter/RuffCheckLinter.php
arcanist/linter/RuffFormatLinter.php
electrum/.ruff.toml

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Harbormaster completed remote builds in B38288: Diff 61518.Sat, Sep 19, 15:22

test lint with included python file

Greesb edited the test plan for this revision. (Show Details)Sat, Sep 19, 15:39

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) If a system is to serve the creative spirit, it must be entirely comprehensible to a single individual.

────────────────────────────────────────────────────────────────────────

major [Functional Correctness]
→ ]8;;vscode://file//work/arcanist/linter/RuffCheckLinter.php:90arcanist/linter/RuffCheckLinter.php:90]8;;

Preserve the file content before Ruff changes the file.

ruff check --fix applies fixes to the target file. After the removed
stdin execution path, line 74 reads $orig after Ruff completes. Line 90
then reads the same fixed content into $new_content. The emitted autofix
has identical original and replacement text, while Ruff has already
modified the working tree outside Arcanist patch handling. Capture the
original content before execution, or use non-mutating diff output.
(docs.astral.sh)

────────────────────────────────────────────────────────────────────────

major [Functional Correctness]
→ ]8;;vscode://file//work/arcanist/linter/RuffFormatLinter.php:88arcanist/linter/RuffFormatLinter.php:88]8;;

Preserve the file content before Ruff formats the file.

ruff format formats path targets in place. Line 72 reads $orig only
after formatting completes, so line 88 reads identical content for
$new_content. Arcanist reports an autofix with no usable replacement and
Ruff modifies the working tree directly. Capture the original content
before execution, or run Ruff in a non-mutating diff mode.
(docs.astral.sh)

────────────────────────────────────────
Review complete
2 findings ✔

Major 2

6 files reviewed:

.arclint
.ruff.toml
arcanist/linter/RuffCheckLinter.php
arcanist/linter/RuffFormatLinter.php
electrum/.ruff.toml
electrum/electrum-abc

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Harbormaster completed remote builds in B38289: Diff 61519.Sat, Sep 19, 15:41

Final update before removing draft status

Harbormaster completed remote builds in B38316: Diff 61562.Tue, Sep 22, 13:29

Remove now useless $orig variable in ruff linters

Harbormaster completed remote builds in B38317: Diff 61563.Tue, Sep 22, 13:30

Greesb published this revision for review.Tue, Sep 22, 13:31

Greesb edited the test plan for this revision. (Show Details)

Greesb edited the summary of this revision. (Show Details)

I'm not sure about all of these changes. What I like:

ruff running on the whole repo and not only electrum abc

What I don't like:

the removal of a the fix option which means more work for the devs

What I'm not sure about:

does this conflict with the other python linters?
what do you mean by "launch the command once for all the files" ? Arcanist will call the linter for each file, even with your change

arcanist/linter/RuffCheckLinter.php
67 ↗	(On Diff #61563)	My understanding is that your are bypassing the linter entirely with this change

This revision now requires changes to proceed.Tue, Sep 22, 13:48

I'm also not a big fan of losing the autofix. I'm used to my arc lint command fixing (almost) all the code in a single pass.
Moving all the config to a standard .ruff.toml file is a big improvement on the other hand (compatibility with IDEs)

In D20675#470199, @Fabien wrote:

I'm not sure about all of these changes. What I like:

ruff running on the whole repo and not only electrum abc

It was already the case given the previous rules in the arcanist linters, just the ruff rules are put in a common file for the whole codebase to share, and easy access to the rules from the IDEs.

What I don't like:

the removal of a the fix option which means more work for the devs

I agree it's not ideal, but it mainly came from the fact that I struggled a _lot_ with how arcanist linters and ruff interacted,
the part I struggled with was parsing the output and displaying it properly via arcanist linters.
The "fix" option can be re-added for both I think (thought it could be quite time-consuming), but since I struggled a lot with that interaction I thought that removing the "fix" option and letting the dev fix the issue itself was better and not that much more work (given the ruff linters are _very_ fast).

What I'm not sure about:

does this conflict with the other python linters?

Not sure what you mean by that, but it shouldn't conflict more than it was already before.

what do you mean by "launch the command once for all the files" ? Arcanist will call the linter for each file, even with your change

Not the best wording on my part. What i meant is that the files are passed with their name to the linter, instead of via stdin,
which was entirely bypassing the file exclusion rules in the new .ruff.toml file I was trying to add.

Greesb added inline comments.Tue, Sep 22, 14:12

arcanist/linter/RuffCheckLinter.php
67 ↗	(On Diff #61563)	At this point the ruff command is already executed, so if the exit code of the ruff command is 0 then that means there is nothing to do (no format or check issue).

In D20675#470204, @PiRK wrote:

I'm also not a big fan of losing the autofix. I'm used to my arc lint command fixing (almost) all the code in a single pass.
Moving all the config to a standard .ruff.toml file is a big improvement on the other hand (compatibility with IDEs)

I'll check if I can re-add the autofix, or at least the output of the checks in the arc lint command.
I removed it at first because I struggled to understand how arc linters worked, but it should probably be doable.

In D20675#470231, @Greesb wrote:

In D20675#470204, @PiRK wrote:

I'm also not a big fan of losing the autofix. I'm used to my arc lint command fixing (almost) all the code in a single pass.
Moving all the config to a standard .ruff.toml file is a big improvement on the other hand (compatibility with IDEs)

I'll check if I can re-add the autofix, or at least the output of the checks in the arc lint command.
I removed it at first because I struggled to understand how arc linters worked, but it should probably be doable.

I can have a quick look and see if there is an easy way to do that. I worked on that linter not too long ago so it's still somewhat fresh in my memory. Somehow our arc lint ruff was able to locate and use electrum's .ruff.toml without having to explicitely add anything to the php code calling the command (in D18906). So I imagine there is a way to also let it use the root .ruff.toml.
I

The main issue is that the files are not passed via their path but via stdin, so the excludes from toml file are not respected.
I think you should be able to fix this in RuffFormatLinter the same way I had to do for RuffCheckLinter in D18906, with --stdin-filename:

diff --git a/arcanist/linter/RuffFormatLinter.php b/arcanist/linter/RuffFormatLinter.php
index 7ca9fceb96..fe3aa62e6c 100644
--- a/arcanist/linter/RuffFormatLinter.php
+++ b/arcanist/linter/RuffFormatLinter.php
@@ -36,11 +36,15 @@ final class RuffFormatLinter extends ArcanistExternalLinter {
   protected function buildFutures(array $paths) {
     $executable = $this->getExecutableCommand();

-    $bin = csprintf('%C %Ls -', $executable, $this->getCommandFlags());
-
     $futures = array();
     foreach ($paths as $path) {
       $disk_path = $this->getEngine()->getFilePathOnDisk($path);
+      $bin = csprintf(
+        '%C %Ls  --stdin-filename %s -',
+         $executable,
+         $this->getCommandFlags(),
+         $disk_path
+      );
       $future = new ExecFuture('%C', $bin);
       /* Write the input file to stdin */
       $input = file_get_contents($disk_path);

I did a few quick tests, and it seems to work. I wasn't able to dedup the include, though (even by setting --force-exclude directly on the command line). It seems you are correct to keep them both in the toml and the .arclint file.

In D20675#470252, @PiRK wrote:
The main issue is that the files are not passed via their path but via stdin, so the excludes from toml file are not respected.
I think you should be able to fix this in RuffFormatLinter the same way I had to do for RuffCheckLinter in D18906, with --stdin-filename:
diff --git a/arcanist/linter/RuffFormatLinter.php b/arcanist/linter/RuffFormatLinter.php
index 7ca9fceb96..fe3aa62e6c 100644
--- a/arcanist/linter/RuffFormatLinter.php
+++ b/arcanist/linter/RuffFormatLinter.php
@@ -36,11 +36,15 @@ final class RuffFormatLinter extends ArcanistExternalLinter {
   protected function buildFutures(array $paths) {
     $executable = $this->getExecutableCommand();

-    $bin = csprintf('%C %Ls -', $executable, $this->getCommandFlags());
-
     $futures = array();
     foreach ($paths as $path) {
       $disk_path = $this->getEngine()->getFilePathOnDisk($path);
+      $bin = csprintf(
+        '%C %Ls  --stdin-filename %s -',
+         $executable,
+         $this->getCommandFlags(),
+         $disk_path
+      );
       $future = new ExecFuture('%C', $bin);
       /* Write the input file to stdin */
       $input = file_get_contents($disk_path);
I did a few quick tests, and it seems to work. I wasn't able to dedup the include, though (even by setting --force-exclude directly on the command line). It seems you are correct to keep them both in the toml and the .arclint file.

It seems like it works, but (with my version at least) there's no diff to be displayed, is that ok for you to just show that the file was reformatted without asking or showing what was modified ?

Because from my understanding with those linters they can either:

Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not
Fix the lint issues and exit with a fail
Fix the lint issues and proceed with the commit/diff automatically

I think I also got lost in all those possible options while also trying to understand how the arcanist linters works 😅

In D20675#470301, @Greesb wrote:

It seems like it works, but (with my version at least) there's no diff to be displayed, is that ok for you to just show that the file was reformatted without asking or showing what was modified ?

Because from my understanding with those linters they can either:

Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not

Fix the lint issues and exit with a fail

Fix the lint issues and proceed with the commit/diff automatically

I think I also got lost in all those possible options while also trying to understand how the arcanist linters works 😅

For some reason the diff is not displayed if you remove setLine(1)->setChar(1) so you can also just restore these. Then another issue I find is that if both ruff format and ruff check find an issue with an auto-fix, only one of them is actually applied, so you have to run arc lint twice. But that's already a problem on master. I don't think there is a workaround for this, it doesn't seem possible to properly chain 2 automatic reformattings in a single pass of arc lint. Switching from having multiple python linters (isort, flake8....) to just ruff check improved that situation quite a lot already.

Try something like this: https://reviews.bitcoinabc.org/D20693 (based on your work with some of the php changes reverted)

In D20675#470311, @PiRK wrote:

In D20675#470301, @Greesb wrote:

It seems like it works, but (with my version at least) there's no diff to be displayed, is that ok for you to just show that the file was reformatted without asking or showing what was modified ?

Because from my understanding with those linters they can either:

Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not

Fix the lint issues and exit with a fail

Fix the lint issues and proceed with the commit/diff automatically

I think I also got lost in all those possible options while also trying to understand how the arcanist linters works 😅

For some reason the diff is not displayed if you remove setLine(1)->setChar(1) so you can also just restore these. Then another issue I find is that if both ruff format and ruff check find an issue with an auto-fix, only one of them is actually applied, so you have to run arc lint twice. But that's already a problem on master. I don't think there is a workaround for this, it doesn't seem possible to properly chain 2 automatic reformattings in a single pass of arc lint. Switching from having multiple python linters (isort, flake8....) to just ruff check improved that situation quite a lot already.

Try something like this: https://reviews.bitcoinabc.org/D20693 (based on your work with some of the php changes reverted)

Yeah that diff is mostly what I have in local that I didn't push yet, I was mostly wondering which of those 3 options I mentioned you would find more acceptable, so that I know when the linters are working as expected.
Because I think all 3 options are possible, both ruff check and ruff format have a --diff option that should be parsable by arc linters (provided I figure out out to parse it properly with ArcanistDiffParser or some other existing parser) and should enable to do the first option, and the other 2 are doable without this --diff option.

Edit: Just in case, i'm speaking about those "3 options":

Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not

Fix the lint issues and exit with a fail

Fix the lint issues and proceed with the commit/diff automatically

Edit 2: I did not have any issue with the chained ruff linters fix, but the ruff check linter do have some errors it can't fix on its own, hence why the parsing output part might be more important specially for this linter.

In D20675#470313, @Greesb wrote:

Yeah that diff is mostly what I have in local that I didn't push yet, I was mostly wondering which of those 3 options I mentioned you would find more acceptable, so that I know when the linters are working as expected.
Because I think all 3 options are possible, both ruff check and ruff format have a --diff option that should be parsable by arc linters (provided I figure out out to parse it properly with ArcanistDiffParser or some other existing parser) and should enable to do the first option, and the other 2 are doable without this --diff option.
Edit: Just in case, i'm speaking about those "3 options":

Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not

Fix the lint issues and exit with a fail

Fix the lint issues and proceed with the commit/diff automatically

Option 1 is not good, imo. You do not want the dev to have a say in code formatting and conventions. You want to be as close as possible to one single deterministic result as possible, so the next diff by another dev does not change the format of the previous dev.

I'm not sure I understand the difference between the next two options. My workflow is usually:

change the code
commit
run arc lint, which shows a diff but I find it hard to read, because no colours, and fixes the code usually without asking for permissions
check with git diff what the linter changed (with colored syntax)
git commit . --amend (I never had any real objections to the linting)
arc diff, which includes a final pass of arc lint to catch those pesky chained auto-formatting quirks (prompts "Amend HEAD with lint patches? [Y/n]", to which I always reply Y, unless the "fix" shows that I somehow messed up a previous step)

Anyhow, this diff should focus on the "no behavior change" of just moving the config to the toml, which the rationale being that it works better with external tools (IDEs...). Changing the behavior is best discussed in another diff with some code, so it can be tested by reviewers.

I think the current behavior of the tool is good, but that's mostly because I'm used to it. If you see a way to improve it, go ahead. I think the goal should be to maximize automation and minimize decision making by devs.

In D20675#470343, @PiRK wrote:

I think the current behavior of the tool is good, but that's mostly because I'm used to it. If you see a way to improve it, go ahead. I think the goal should be to maximize automation and minimize decision making by devs.

Yes

arcanist/linter/RuffCheckLinter.php
67 ↗	(On Diff #61563)	Oh so ruff is this kind of linter that returns an error code if it executed properly but found issues, got it. I missed the change in shouldExpectCommandErrors above

In D20675#470343, @PiRK wrote:

I think the current behavior of the tool is good, but that's mostly because I'm used to it. If you see a way to improve it, go ahead. I think the goal should be to maximize automation and minimize decision making by devs.

Sounds good to me. I'll focus on reproducing the current behavior for now then.

arcanist/linter/RuffCheckLinter.php
67 ↗	(On Diff #61563)	Yes kinda, though this behavior is configurable with ruff's command line arguments (`--exit-zero`, `--exit-non-zero-on-fix` and `--exit-non-zero-on-format`)

Update ruff linters to fix issues whenever they can and return an error message to arc lint, when an issue has been fixed or found (since not all issues can be fixe automatically).

Harbormaster completed remote builds in B38349: Diff 61608.Thu, Sep 24, 08:59

Greesb edited the summary of this revision. (Show Details)Thu, Sep 24, 09:02

Both linters should fix all issue they find when using arc lint, there are no detailed message shown except that there were code-style violations on a file.
Here's an example with a random file:

λ arc lint
>>> Lint for electrum/electrum-abc:


   Auto-Fix  (RUFF-FORMAT) Code style violation
    '/home/greesb/dev/bitcoin-abc/electrum/electrum-abc' has code style
    errors.

   Auto-Fix  (RUFF-CHECK) Code style violation
    '/home/greesb/dev/bitcoin-abc/electrum/electrum-abc' has code style
    errors.
OKAY  No lint messages.

@PiRK @Fabien It should be good now, I have re-added autofix to both linters.

@bot ai-review

teamcity awarded a token.Thu, Sep 24, 13:47

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Your tests are like unicorns: frequently referenced, rarely seen.

────────────────────────────────────────────────────────────────────────

major [Data Integrity & Integration]
→ ]8;;vscode://file//work/arcanist/linter/RuffFormatLinter.php:62arcanist/linter/RuffFormatLinter.php:62-63]8;;

Keep formatting changes under Arcanist’s patch controls.

The inherited buildFutures() passes the on-disk file path to `ruff
format`, which formats the file in place. Arcanist therefore cannot honor
--never-apply-patches or present the formatting change as a patch before
it happens. Restore stdin-based formatting and provide replacement text to
Arcanist, or use a non-writing format check. (raw.githubusercontent.com)

────────────────────────────────────────────────────────────────────────

major [Functional Correctness]
→ ]8;;vscode://file//work/arcanist/linter/RuffFormatLinter.php:68arcanist/linter/RuffFormatLinter.php:68-69]8;;

Treat Ruff format exit status 2 as a failure.

If Ruff cannot format because of an invalid configuration, invalid option,
or internal error, it exits with status 2. This branch instead emits an
SEVERITY_AUTOFIX message. Arcanist does not count that message as an
error or warning, so arc lint can return success without formatting the
file. Report status 2 as an error rather than an autofix. (docs.astral.sh)

────────────────────────────────────────────────────────────────────────

major [Functional Correctness]
→ ]8;;vscode://file//work/arcanist/linter/RuffCheckLinter.php:69arcanist/linter/RuffCheckLinter.php:69-70]8;;

Report unfixed violations and Ruff failures as errors.

If Ruff exits with status 1 for a violation that --fix cannot resolve,
or status 2 for a command or configuration failure, this path emits only
an SEVERITY_AUTOFIX message. Arcanist does not count that unresolved
message as an error or warning, so arc lint can return success.
Distinguish fixed files from remaining diagnostics and abnormal exits;
give the latter error severity. (docs.astral.sh)

────────────────────────────────────────────────────────────────────────

major [Data Integrity & Integration]
→ ]8;;vscode://file//work/arcanist/linter/RuffCheckLinter.php:64arcanist/linter/RuffCheckLinter.php:64]8;;

Do not apply lint fixes outside Arcanist’s patch controls.

The inherited buildFutures() passes the on-disk file path to Ruff. With
--fix, Ruff changes that file before Arcanist can honor
--never-apply-patches or review a patch. Restore stdin-based processing
and return a replacement-text patch to Arcanist, or use a non-writing
check. (raw.githubusercontent.com)

────────────────────────────────────────
Review complete
4 findings ✔

Major 4

5 files reviewed:

.arclint
.ruff.toml
arcanist/linter/RuffCheckLinter.php
arcanist/linter/RuffFormatLinter.php
electrum/.ruff.toml

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Fabien requested changes to this revision.Thu, Sep 24, 14:17

Fabien added inline comments.

.arclint
26 ↗	(On Diff #61608)	This has a performance drawback, maybe it's no big deal but it's worth checking. With this change the linter is called for each python file, launching a new process (python) that will launch a new script (ruff) which will be immediately aborted because the linter will filter the file out. Twice because linter + check.
arcanist/linter/RuffCheckLinter.php
115 ↗	(On Diff #61608)	Not showing the changes is a no-go. A linter should not produce code change that is not shown to the dev for validation. There are options I didn't know about or maybe didn't exist when it was first written, like --show-fixes or --diff and even an --output-format. It should be possible to use these options so arcanist still shows a proper diff of the fixed changes, while still autofixing.

This revision now requires changes to proceed.Thu, Sep 24, 14:17

Fabien added inline comments.Thu, Sep 24, 14:19

arcanist/linter/RuffCheckLinter.php
69 ↗	(On Diff #61608)	The AI is correct on this one: if the code is zero, no violation, your code is correct. But if the code is 2 then the linter did not run and then the following code is wrong. You'll need to return false in this case.

Greesb planned changes to this revision.Thu, Sep 24, 15:50

Greesb added inline comments.

.arclint
26 ↗	(On Diff #61608)	I didn't know that about the arc linters 🤔 It will most likely have a small impact, but probably very small given ruff will exit very fast after checking that the file is excluded.
arcanist/linter/RuffCheckLinter.php
69 ↗	(On Diff #61608)	Indeed nice catch, will be fixed on next push.
115 ↗	(On Diff #61608)	I'll see what I can do to show them, there are a lot of ways to do it indeed. Just thought it wouldn't matter since the changes would still need to be applied anyway.

PiRK added inline comments.Thu, Sep 24, 19:09

.arclint
26 ↗	(On Diff #61608)	I think dropping the excludes is not to much of a perf hit. It would be a bigger problem to drop the includes (they serve as first filter, which drops already most of the codebase). We rarely touch those excluded files. The benefit is a bit less duplication. We need those excludes to be in the toml file if we want external tools to not mess with excluded files.

PiRK added inline comments.Thu, Sep 24, 19:11

.arclint
26 ↗	(On Diff #61608)	Alternatively we can duplicate them to have the best perf, but then we need a comment in the toml file mentioning that .arclint needs to be updated whenever new excludes are added.

Fabien added inline comments.Fri, Sep 25, 07:00

.arclint
26 ↗	(On Diff #61608)	I didn't know that about the arc linters 🤔 It will most likely have a small impact, but probably very small given ruff will exit very fast after checking that the file is excluded. The cost is not running ruff, but rather launching a new python process for each file (even if it does nothing).

I ran the linter on all files, both on master and on this patch. On both I modified the .arclint file to only keep the generated, ruff-format and ruff-check linters. My governor is in eco mode to reduce the imapct of the CPU boost.
I noticed 3 files from electrum/scripts have autofixes with this patch (but not on master, so the rules might be different). I excluded the electrum/script/*.py files from the run with this patch so it doesn't interfere with the measurement.

On master:

$ time arc lint --everything
 OKAY  No lint messages.

real	0m1,096s
user	0m2,249s
sys	0m3,068s

On arcpatch-D20675:

$ time arc lint --everything
 OKAY  No lint messages.

real	0m8,509s
user	0m11,085s
sys	1m27,875s

So it's roughly 8,5x slower to exclude the files from ruff instead of arcanist on my machine, and much worst on machines with less CPU cores as you can see from the sys measurement.
Let's keep the exclusion in arcanist.

In D20675#470704, @Fabien wrote:
I ran the linter on all files, both on master and on this patch. On both I modified the .arclint file to only keep the generated, ruff-format and ruff-check linters. My governor is in eco mode to reduce the imapct of the CPU boost.
I noticed 3 files from electrum/scripts have autofixes with this patch (but not on master, so the rules might be different). I excluded the electrum/script/*.py files from the run with this patch so it doesn't interfere with the measurement.

On master:
$ time arc lint --everything
 OKAY  No lint messages.

real	0m1,096s
user	0m2,249s
sys	0m3,068s
On arcpatch-D20675:
$ time arc lint --everything
 OKAY  No lint messages.

real	0m8,509s
user	0m11,085s
sys	1m27,875s
So it's roughly 8,5x slower to exclude the files from ruff instead of arcanist on my machine, and much worst on machines with less CPU cores as you can see from the sys measurement.
Let's keep the exclusion in arcanist.

Damn I didn't think it would be that much, even with a new process for each files. I'll keep the exclude in arclint and add a comment on both arclint and .ruff.toml to mention the link between both then, thanks for taking the measurements !

Restore diff output in arcanist (with autofix still enabled) and move ruff linters common code to an AbstractRuffExternalLinter

Harbormaster completed remote builds in B38438: Diff 61752.Sat, Oct 3, 16:14

Greesb marked 6 inline comments as done.Sat, Oct 3, 16:15

Just a rebase on master

Harbormaster completed remote builds in B38439: Diff 61753.Sat, Oct 3, 16:16

run arc liberate

Harbormaster completed remote builds in B38440: Diff 61754.Sat, Oct 3, 16:17

Remove unnecessary reference to pinterest repository in comments

Harbormaster completed remote builds in B38441: Diff 61755.Sat, Oct 3, 16:35

@Fabien @PiRK Should be good now, there's autofix and arcanist still shows the diff of the things it autofixed. And I also took the liberty to make an abstract class for ruff executable and output parsing to avoid duplicating code in both ruff format and ruff check linters.

Everything seems to be working as expected, I'm just trying to figure what rule changed for electrum/scripts to now be changed by arc lint --everything (looks like now it can better detect first-party includes.

@bot ai-review

teamcity updated a token.Mon, Oct 5, 10:48

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Goodbye, diff induced despair.

────────────────────────────────────────────────────────────────────────

major [Functional Correctness]
→ ]8;;vscode://file//work/arcanist/linter/AbstractRuffExternalLinter.php:35arcanist/linter/AbstractRuffExternalLinter.php:35-40]8;;

--diff makes ruff check hide violations that it cannot fix.

buildFutures() adds --diff to every Ruff command. For ruff check,
--diff implies --fix-only. Ruff then prints only the diff for fixable
violations. Ruff does not report unfixable violations such as F821
(undefined name), F811, or E999.

parseLinterOutput() builds lint messages only from that diff. As a
result, ruff-check never reports unfixable errors, and arc lint passes
on code with real defects. Before this change, RuffCheckLinter did not
pass --diff.

Use --diff only for ruff format. For ruff check, choose one of these
fixes:
- Run a second pass with --output-format json and report each violation
it returns.
- Keep the old approach, which wrote the fixed content to stdout.


Sketch: make the extra flags a subclass hook

-        '%C %Ls --diff --stdin-filename %s -',
+        '%C %Ls --stdin-filename %s -',
         $executable,
-        $this->getCommandFlags(),
+        array_merge($this->getCommandFlags(), $this->getOutputFlags()),
         $disk_path

RuffFormatLinter::getOutputFlags() returns array('--diff').
RuffCheckLinter keeps a separate output path that reports unfixable
diagnostics.

────────────────────────────────────────
Review complete
1 finding ✔

Major 1

7 files reviewed:

.arclint
.ruff.toml
arcanist/__phutil_library_map__.php
arcanist/linter/AbstractRuffExternalLinter.php
arcanist/linter/RuffCheckLinter.php
arcanist/linter/RuffFormatLinter.php
electrum/.ruff.toml

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

PiRK added inline comments.Mon, Oct 5, 10:53

.ruff.toml
49	you can probably remove this line, we don't include the `electrum` root in any part of the monorepo. It does not have an __init__.py
electrum/.ruff.toml
6	this was wrong, fixed by the diff

The AI review is right, the linter now longer reports some obvious issues.

On master:

$ git diff
diff --git a/electrum/electrum-abc b/electrum/electrum-abc
index 8e2f638b0b..72ef045270 100755
--- a/electrum/electrum-abc
+++ b/electrum/electrum-abc
@@ -183,6 +183,7 @@ def restore_wallet(
 ) -> AbstractWallet:
     """Restore an existing wallet"""
     text = simple_config.get("text").strip()
+    print(txet)
     passphrase = simple_config.get(ConfigKeys.PASSPHRASE)
     password = None
     if keystore.is_private(text):
$ arc lint
 Exception
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      ruff check --fix --ignore 'A003,E203,E303,E305,E501,C901' --select 'E,F,A,C,I,ASYNC,FLY,PLE,B006,B008,B912,RUF064,UP031,UP032' --stdin-filename /home/pierre/dev/bitcoin-abc/electrum/electrum-abc -

      STDOUT
      #!/usr/bin/env python3
      # -*- mode: python3 -*-
      #
      # Electrum ABC - lightweight eCash client
      # Copyright (C) 2020 The Electrum ABC developers
      # Copyright (C) 2011 thomasv@gitorious
      #
      # Electron Cash - lightweight Bitcoin Cash client
      # Copyright (C) 2017-2020 The Electron Cash Developers
      #
      # Permission is hereby granted, free of charge, to any person
      # obtaining a copy of this software and associated documentation files
      # (the "Software"), to deal in the Software without restriction,
      # including without limitation the rights to use, copy, modify, merge,
      # publish, distribute, sublicense, and/or sell copies of the Software,
      # and to permit persons to whom the Software is furnished to do so,
      # subject to the following conditions:
      #
      # The above copyright notice and this permission notice shall be
      # included in all copies or substantial portions of the Software.
      #
      # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
      # EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIE... (20,876 more bytes) ...

      STDERR
      F821 Undefined name `txet`
         --> electrum/electrum-abc:186:11
          |
      184 |     """Restore an existing wallet"""
      185 |     text = simple_config.get("text").strip()
      186 |     print(txet)
          |           ^^^^
      187 |     passphrase = simple_config.get(ConfigKeys.PASSPHRASE)
      188 |     password = None
          |

      Found 1 error.

(Run with `--trace` for a full exception trace.)

On this diff, with the same bug:

$ arc lint
 OKAY  No lint messages.

This revision now requires changes to proceed.Mon, Oct 5, 11:01

This is good work, and the refactor into a base class makes sense. I would just recommend splitting this work into two or 3 smaller diffs so the non-controversial changes can get landed quickly and there is a separate rationale for every piece of the work:

extract rules to .ruff.toml, minimzing changes to the the php (some are necesarry, D20693), while avoiding any potential change in behavior (same command line args)
extract the common stuff to AbstractRuffExternalLinter, pure refactor with no change of behavior
change command line args, --diff etc... (for this part the rationale is still not clear to me)

In D20675#471765, @PiRK wrote:

This is good work, and the refactor into a base class makes sense. I would just recommend splitting this work into two or 3 smaller diffs so the non-controversial changes can get landed quickly and there is a separate rationale for every piece of the work:

extract rules to .ruff.toml, minimzing changes to the the php (some are necesarry, D20693), while avoiding any potential change in behavior (same command line args)

extract the common stuff to AbstractRuffExternalLinter, pure refactor with no change of behavior

change command line args, --diff etc... (for this part the rationale is still not clear to me)

Yeah fair point, I got a bit lost with all those changes while also learning the limitations of arcanist linters. I made https://reviews.bitcoinabc.org/D20762 to just move the rules, and will work on improving the linters php code in this diff or another one afterwards.
@PiRK

Greesb planned changes to this revision.Mon, Oct 5, 14:35

Made https://reviews.bitcoinabc.org/D20767 for the abstract class changes, I don't think the diff part is needed at all anymore, so i'll close this diff.

ruff: move rules from .arclint to .ruff.toml
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 61755

.arclint

.ruff.toml

arcanist/__phutil_library_map__.php

arcanist/linter/AbstractRuffExternalLinter.php

arcanist/linter/RuffCheckLinter.php

arcanist/linter/RuffFormatLinter.php

electrum/.ruff.toml

ruff: move rules from .arclint to .ruff.tomlAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 61755

.arclint

.ruff.toml

arcanist/__phutil_library_map__.php

arcanist/linter/AbstractRuffExternalLinter.php

arcanist/linter/RuffCheckLinter.php

arcanist/linter/RuffFormatLinter.php

electrum/.ruff.toml

ruff: move rules from .arclint to .ruff.toml
AbandonedPublic
Actions

Revision Contents
Changeset List