Skip to content

GitHub Actions: Run Clippy on stubs#1011

Merged
petertseng merged 7 commits intoexercism:masterfrom
petertseng:clippy-stub
Nov 18, 2020
Merged

GitHub Actions: Run Clippy on stubs#1011
petertseng merged 7 commits intoexercism:masterfrom
petertseng:clippy-stub

Conversation

@petertseng
Copy link
Copy Markdown
Member

Fails the build if any stubs have Clippy errors.
Does not fail the build if stubs have Clippy warnings, but does display
them (and there are a number of them).

The author of this PR is not sure we will ever get around to fixing all
the Clippy warnings, but supposes that it is at least beneficial to make
sure we add no new Clippy errors.

1/3 of #659

(By the way, ensure-stubs-compile already had a bad mix of spaces and
tabs and this commit did not help any in making it better. The author's
text editor cannot easily put tabs in shell scripts)


Here is the list of warnings emitted from all the stubs (well, each prints twice so you can probably consider each count halved)

grep -B1 src/lib.rs gha-clippy-log.txt | sed 's/^20..-..-..T..:..:..........Z //' | grep 'warning\|error' | sort | uniq -c | sort -nr
      4 warning: useless use of `format!`
      2 warning: you should consider adding a `Default` implementation for `SimpleLinkedList<T>`
      2 warning: you should consider adding a `Default` implementation for `School`
      2 warning: you should consider adding a `Default` implementation for `Robot`
      2 warning: you should consider adding a `Default` implementation for `Reactor<T>`
      2 warning: you should consider adding a `Default` implementation for `LinkedList<T>`
      2 warning: you should consider adding a `Default` implementation for `Forth`
      2 warning: you should consider adding a `Default` implementation for `Fizzy<T>`
      2 warning: you should consider adding a `Default` implementation for `BowlingGame`
      2 warning: unneeded unit return type
      2 warning: item `SimpleLinkedList<T>` has a public `len` method but no corresponding `is_empty` method
      2 warning: item `LinkedList<T>` has a public `len` method but no corresponding `is_empty` method
      2 warning: defining a method called `next` on this type; consider implementing the `std::iter::Iterator` trait or choosing a less ambiguous name

Comment thread _test/ensure-stubs-compile.sh Outdated
Fails the build if any stubs have Clippy errors.
Does not fail the build if stubs have Clippy warnings, but does display
them (and there are a number of them).

The author of this PR is not sure we will ever get around to fixing all
the Clippy warnings, but supposes that it is at least beneficial to make
sure we add no new Clippy errors.

1/3 of #659

(By the way, ensure-stubs-compile already had a bad mix of spaces and
tabs and this commit did not help any in making it better. The author's
text editor cannot easily put tabs in shell scripts)
@coriolinus
Copy link
Copy Markdown
Member

WRT tabs and spaces. I hadn't noticed, but I would not object to a global replacement of tabs with spaces in our script files.

@petertseng petertseng merged commit 465ac7e into exercism:master Nov 18, 2020
@petertseng petertseng deleted the clippy-stub branch November 18, 2020 09:45
petertseng added a commit that referenced this pull request Nov 20, 2020
It's understandable that clippy says the `next` here conflicts with
Iterator's `next`, but I really think that's the best name for it unless
we go with something like `forward` and `back`.

There is one other example of Cursors on the internet:
https://contain-rs.github.io/linked-list/linked_list/struct.Cursor.html
This one also uses `next`, so it seems they did not find a better
solution to this than we could.

Helps address #1011
Helps address #1012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants