Added clippy check. #65

5c077m4n · 2020-11-21T19:49:56Z

No description provided.

roee-shapira-tikal · 2020-11-21T20:22:05Z

@imsnif not too bad, right?

roee-shapira-tikal · 2020-11-21T20:23:29Z

.github/workflows/rust.yml

+ with:
+ toolchain: stable
+ components: clippy
+ - uses: actions-rs/clippy@master


@imsnif This version needs to be changed asap.

Sorry, I'm not sure I understand?

The master tag as it's not versioned.
But from what I see it's not really being developed actively

khs26 · 2020-11-21T21:31:15Z

src/terminal_pane/scroll.rs

+ while let Some((current_index, current_line)) = indices_and_canonical_lines.next() {
+ if current_index == canonical_line_cursor_position {
+ y += current_line.wrapped_fragments.len() - line_wrap_cursor_position;
+ break;
+ } else {
+ y += current_line.wrapped_fragments.len();


This seems much nicer

khs26 · 2020-11-21T21:31:49Z

src/screen.rs

@@ -67,6 +67,7 @@ pub enum ScreenInstruction {
 ClearScroll,
 CloseFocusedPane,
 ToggleActiveTerminalFullscreen,
+ #[allow(dead_code)]


I'm not sure about adding warning suppressions (similarly for the other allow directives)

I feel the same... @5c077m4n - could you share the reasoning here?

ah, oops - didn't realize this was already changed

imsnif · 2020-11-23T15:22:48Z

@5c077m4n - looking good! I'm fine with everything here. What else needs to be done? Do you also want to take care of some of the warnings we've got?

imsnif · 2020-11-23T18:37:00Z

Also, could you merge from main? There are some changes there that might need to run through these tests

roee-shapira-tikal · 2020-11-23T20:32:47Z

src/screen.rs

@@ -142,7 +142,7 @@ impl Screen {
 let pid = new_pids.next().unwrap(); // if this crashes it means we got less pids than there are panes in this layout
 let mut new_terminal = TerminalPane::new(
 *pid,
- self.full_screen_ws.clone(),
+ self.full_screen_ws,


@imsnif clippy really does not like the .clone() here, is it necessary?

I think this is Copy, so no need for sure. It's probably a leftover from a refactor.

imsnif · 2020-11-24T08:24:05Z

@5c077m4n - so is this ready?

imsnif · 2020-11-24T08:41:58Z

src/utils/logging.rs

-fn atomic_create_file(file_name: &str) {
- let _ = fs::OpenOptions::new().create(true).open(file_name);
+fn atomic_create_file(file_name: &str) -> io::Result<fs::File> {
+ fs::OpenOptions::new().create(true).open(file_name)


@5c077m4n - the previous way was actually intentional. This is "atomic" create file, meaning that it's okay for it to fail if the file already exists.

Reverted. Sorry 🤦

roee-shapira-tikal · 2020-11-24T08:48:20Z

This is nearly done, there are just a couple of more minor changes that you might want to look at yourself @imsnif (like src/main.rs:309:41)

imsnif · 2020-11-24T08:51:15Z

So... is this ready for review or not?

roee-shapira-tikal · 2020-11-24T08:52:15Z

Yep, now it is

imsnif

Really great work on this! I imagine this was not trivial.

I left some general comments. I'm looking into the stdin error (it might be the cause of some of our troubles). But until then (and on top of my comments in the code): why is the build not failing? It has two errors and several warnings. Shouldn't it be red?

imsnif · 2020-11-24T11:10:40Z

src/terminal_pane/terminal_pane.rs

- _ => {}
+ if let Some(&25) = params.get(0) {
+ self.scroll.show_cursor();
+ self.mark_for_rerender();
 };
 }
 } else if c == 'r' {


@5c077m4n - I can't comment on the lines below directly for some reason, but can we disable this clippy check? The one with the errors about the identical ifs. I would really like to leave it like this for now. The comments are super important and this block is generally very much in-flux as we keep reverse-engineering the VT100 protocol.

imsnif · 2020-11-24T11:12:09Z

src/utils/logging.rs

 }

-pub fn delete_log_file() -> io::Result<()> {
+pub fn _delete_log_file() -> io::Result<()> {


The functions in this file are general utility functions. Can we add exceptions for them so that we don't have to prefix them with underscores? It would just be confusing and annoying while debugging :)

roee-shapira-tikal · 2020-11-24T11:55:58Z

@imsnif the tests are not failing because this clippy github actions is not really being developed, I've looked some other possibility but this still looks better
And about the clippy exceptions there are annotations that can disable the warnings (#[allow(clippy::...)])

imsnif · 2020-11-24T12:18:05Z

@imsnif the tests are not failing because this clippy github actions is not really being developed, I've looked some other possibility but this still looks better

So... I guess this solution doesn't do the job for us? If we can't see the failures on PRs, then this essentially will not be part of our CI. Or am I missing something?

And about the clippy exceptions there are annotations that can disable the warnings (#[allow(clippy::...)])

Cool - so can we add them?

imsnif · 2020-11-24T12:19:10Z

Also (if we're going forward with this) we should either address the warnings or have them not appear inside the code-browsing. Because it's hard to review PRs with them.

roee-shapira-tikal · 2020-11-24T12:20:38Z

Sure, I'll do what you asked tonight/tomorrow morning @imsnif

imsnif · 2020-11-24T12:23:45Z

Sure, I'll do what you asked tonight/tomorrow morning @imsnif

Awesome. But just to stress: we need some sort of failure in our CI for this to be meaningful. I don't mind it even being a hack, like a test-case running clippy or whatever :)

roee-shapira-tikal · 2020-11-24T12:32:58Z

😎

5c077m4n · 2020-11-26T20:46:37Z

@imsnif after some research I found that there are three possibilities to clippy check in the ci:

an action that can't check PRs
an action that doesn't fail the build
an action that does not show the errors on the pr reviews
Other than creating a brand new action I don't really an easy way of using this tool...

imsnif · 2020-11-26T21:13:14Z

I see... thanks for all the research! Does number 3 still check PRs and fail the build though?

5c077m4n · 2020-11-26T21:37:03Z

Yep, that's what's configured for now

imsnif · 2020-11-26T22:32:16Z

Alright, so maybe it can be a good compromise?

5c077m4n · 2020-11-27T05:00:51Z

Great :) in that case this branch is ready for your review @imsnif

imsnif

Great work as always. Thanks for doing this! I think we will all appreciate it :)

Roee Shapira added 3 commits November 21, 2020 21:37

Added clippy check.

a35b084

Changed step name.

5c1c051

Used the suggested ci config.

9b7f31e

5c077m4n marked this pull request as draft November 21, 2020 20:04

Roee Shapira added 3 commits November 21, 2020 22:13

Some more clippy fixes.

f223580

Some more clippy fixes.

6475f98

More clippy fixes.

b0081bb

roee-shapira-tikal reviewed Nov 21, 2020

View reviewed changes

khs26 reviewed Nov 21, 2020

View reviewed changes

Removed allow annotations.

602e38a

5c077m4n marked this pull request as ready for review November 23, 2020 17:26

Roee Shapira added 3 commits November 23, 2020 22:04

Merge remote-tracking branch 'upstream/main' into add-clippy-to-ci

2b62e21

Minor lint edit.

fa5b91d

More clippy fixes.

c232f13

roee-shapira-tikal reviewed Nov 23, 2020

View reviewed changes

Roee Shapira added 4 commits November 23, 2020 22:36

Ran cargo fmt.

8f00472

More clippy fixes.

8eefad3

More clippy fixes.

86cc0ae

More clippy fixes.

225554c

Roee Shapira added 3 commits November 24, 2020 10:31

More clippy fixes.

6427d11

More clippy fixes.

182bc1d

More clippy fixes.

a1427bd

imsnif reviewed Nov 24, 2020

View reviewed changes

Reverted fix.

14ec65c

imsnif requested changes Nov 24, 2020

View reviewed changes

Roee Shapira added 8 commits November 25, 2020 09:57

Code review edits.

ef60139

Code review edits.

fb721f1

Code review edits.

50d6374

CI update.

f1fbed5

CI update.

b692336

CI update.

271fa89

Merged with upstream/master.

4b623c4

Added clippy warn so wip won't fail ci.

20d1fc6

Cargo fmt.

93b48eb

imsnif approved these changes Nov 28, 2020

View reviewed changes

imsnif merged commit 9ad7b8a into zellij-org:main Nov 28, 2020

cswinter mentioned this pull request May 5, 2022

called Result::unwrap() on an Err value: "SendError(..)" in lib.rs:347:22 #1380

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added clippy check. #65

Added clippy check. #65

5c077m4n commented Nov 21, 2020

roee-shapira-tikal commented Nov 21, 2020

roee-shapira-tikal Nov 21, 2020 •

edited

Loading

imsnif Nov 23, 2020

roee-shapira-tikal Nov 23, 2020

khs26 Nov 21, 2020

khs26 Nov 21, 2020

imsnif Nov 23, 2020

imsnif Nov 23, 2020

imsnif commented Nov 23, 2020

imsnif commented Nov 23, 2020

roee-shapira-tikal Nov 23, 2020

imsnif Nov 23, 2020

roee-shapira-tikal Nov 23, 2020

imsnif commented Nov 24, 2020

imsnif Nov 24, 2020

roee-shapira-tikal Nov 24, 2020 •

edited

Loading

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

imsnif left a comment

imsnif Nov 24, 2020

imsnif Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

5c077m4n commented Nov 26, 2020

imsnif commented Nov 26, 2020

5c077m4n commented Nov 26, 2020 •

edited

Loading

imsnif commented Nov 26, 2020

5c077m4n commented Nov 27, 2020

imsnif left a comment

Added clippy check. #65

Added clippy check. #65

Conversation

5c077m4n commented Nov 21, 2020

roee-shapira-tikal commented Nov 21, 2020

roee-shapira-tikal Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsnif commented Nov 23, 2020

imsnif commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsnif commented Nov 24, 2020

Choose a reason for hiding this comment

roee-shapira-tikal Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

imsnif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

imsnif commented Nov 24, 2020

roee-shapira-tikal commented Nov 24, 2020

5c077m4n commented Nov 26, 2020

imsnif commented Nov 26, 2020

5c077m4n commented Nov 26, 2020 • edited Loading

imsnif commented Nov 26, 2020

5c077m4n commented Nov 27, 2020

imsnif left a comment

Choose a reason for hiding this comment

roee-shapira-tikal Nov 21, 2020 •

edited

Loading

roee-shapira-tikal Nov 24, 2020 •

edited

Loading

5c077m4n commented Nov 26, 2020 •

edited

Loading