Clippy pendantic and more refactors by soruh · Pull Request #219 · lycheeverse/lychee

soruh · 2021-04-12T22:12:32Z

Check the code with clippy::pedantic, fix most of it's complaints and ignore the rest (mostly documentation tasks).
Manually converted some code into using methods on Option and Result instead of matches.

P.S.: my rustfmt seems to have collapsed some uses and comment, if you dislike this I can try to undo that manually.

P.P.S.: sorry for creating another relatively large PR :)

soruh · 2021-04-12T22:19:14Z

Ah it seems I accidentally used some nightly features

src/extract.rs

mre · 2021-04-13T08:55:33Z

src/bin/lychee/main.rs

-            let bar = ProgressBar::new(links.len() as u64)
-                .with_style(ProgressStyle::default_bar().template(
+    let with_progress = !cfg.no_progress;
+    let pb = with_progress.then(|| {


Hum... I find the old version easier to understand. 🤔

I can see that I'll revert it when I rebase on the new master when #208 is merged.

#208 is merged now. 😄

src/bin/lychee/main.rs

mre · 2021-04-13T08:57:12Z

src/client.rs

+            .and_then(|token| {
+                (!token.is_empty()).then(|| Github::new(user_agent, Credentials::Token(token)))
+            })
+            .transpose()?;


Aha! transpose does make sense here.

Yeah I was so happy when I found out about transpose

mre · 2021-04-13T08:59:31Z

Great improvements! Thanks a lot for this.
I'd like to wait with the merge until #208 is in. Most likely you'd have to rebase your changes on top of master once that is done. Hope that's okay for you.

soruh · 2021-04-13T18:02:57Z

Seems like a good Idea although doing this again on the new master might actually be less work than rebasing.

lebensterben · 2021-04-13T19:10:15Z

There's no need to merge this.
I've already applied clippy::pendantic in #208.

soruh · 2021-04-13T19:31:51Z

Which is why the plan is to rebase on top of your PR.
Also as far I can see you didn't actually enable clippy::pedantic but only applied it's suggestions which means that it will not be checked against by CI.

lebensterben · 2021-04-13T20:50:31Z

@soruh
That's true. But you shouldn't rebase on top of my PR because you will see many rebase conflicts. It's much easier just to discard this one and start a new PR which adds the clippy attributes.
P.S. I've enabled both clippy::pedantic and clippy::all.

mre · 2021-04-22T20:35:46Z

@soruh, would you like to rebase on top of the current master, create a new PR with the improvements or are all those changes covered by #208 already? 😊

soruh · 2021-04-22T21:57:56Z

I think there are some additional changes, I'll try to rebase it when I find the time and will make a new PR if there are significant changes left / close this one if there aren't.

mre · 2021-04-22T22:09:33Z

Thanks!

…:from lowercase

soruh · 2021-04-29T21:25:39Z

I just rebased my code onto master, there is not a huge amount left but still enough for keeping this open I think.

I re-added my old solution of converting file extensions to lower-case before checking them. By now I have realized that this is probably not the optimal solution (as is always heap allocates). A version of to_lowercase that returns a Cow or just checking for the uppercase variants of the extensions directly may be preferable.

I personally like the .not().then(..) pattern but am of course open to change it to something more traditional if preferred.

There a a few things I would like an opinion on (annotated with REVIEW).

lychee-bin/src/main.rs

lebensterben · 2021-04-29T23:02:04Z

lychee-bin/src/main.rs

-        None => tokio::runtime::Runtime::new()?,
-    };
+        None => tokio::runtime::Runtime::new(),
+    }?;


ok. but easy to miss when reading.

We are creating a runtime in both cases and are checking if that failed after which I find more readable.

the ? operator in }?; is quite easy to miss.

lebensterben · 2021-04-29T23:05:18Z

lychee-bin/src/options.rs

                return match e.kind() {
                    ErrorKind::NotFound => Ok(None),
-                    _ => Err(Error::from(e)),
+                    _ => Err(e.into()),


No I deliberately avoid using Into::into().
It's hard to figure out what it is converting to.

We are calling .into() inside of an Err(...), the Error:: doesn't give you any new information here and just makes the more verbose.

How do you figure out what's the type of E in a Result<T, E>?
Error::from(e) clearly shows E is of type Error, while e.into() has no such information.

lebensterben · 2021-04-29T23:06:59Z

lychee-bin/src/main.rs

-    let pb = if cfg.no_progress {
-        None
-    } else {
+    let pb = cfg.no_progress.not().then(|| {


Does this increase any readability or performance?

I think the functional style is clearer but I agree that the .not() may not be (see my original comment).

The issue is not with not, the main issue is the functional style.
std::option::Option::then() is a fairly new addition. If it's the more idiomatic way of handling Boolean conditionals, it'd be added much earlier.

lebensterben · 2021-04-29T23:08:19Z

lychee-lib/src/client.rs

+        let github_token = self
+            .github_token
+            .as_ref()
+            .and_then(|token| {


Again.
When I refactored these code, I deliberately avoided this kind of pattern.

I agree that the not() should be a ! but think this is way more readable than the Some(ref token) if !token.is_empty() => Some( incantation it replaced. I agree that this is a matter of opinion though.

No.

I deliberately keep Some(ref token). this make it very clear of the type signature.
( Calling as_ref() has the same effect. )

The not readable part is the nested and_then and then. Unless you add explicit type signature, it's less clear. But if you did, it's too verbose.

lychee-lib/src/client.rs

lebensterben · 2021-04-29T23:12:40Z

lychee-lib/src/extract.rs

        (attr_name, elem_name),
+        // TODO: re-enable once `or-patterns` become stable in Rust 1.53
+        // ("href" | "src" | "srcset" | "cite", _) | ("data", "object") | ("onhashchange", "body")
        ("href", _)


When I refactored this part, it's requested to keep the current form for readability.

lebensterben · 2021-04-29T23:18:50Z

lychee-lib/src/extract.rs

        // assert_eq!(FileType::from(Path::new("/")), FileType::Plaintext);
+        // assert_eq!(FileType::from("test"), FileType::Plaintext);
        assert_eq!(FileType::from("test.md"), FileType::Markdown);
+        assert_eq!(FileType::from("TEST.MD"), FileType::Markdown);


-1
On modern file system it's case sensitive.
According to mimeinfo of text/markdown, valid extension is one of .md .mkd and .markdown.

If a .MD extension is recognized as markdown, it IS A BUG.

Are you saying that handling an html file incorrectly because it is called "index.HTML" is okay? Or are you only referring to Markdown files? In that case I guess If we do what I proposed in my comment above we can only check if certain extension match insensitively, I don't see how handling files with extensions that are not technically in spec is an improvement

I'm not talking about the REVIEW comment.

What I'm taking is the additional assert_eq tests you added are not sound. They should be removed.

Ah, that makes sense, are you okay with changing that to a ".HTML" test or are uppercase extension not technically in-spec in general?

To incorporate them is very complicated.
For modern Linux/Unix/FreeBSD systems, the file system should be case-sensitive. Thus these are unknown schemes and are recognized as planttext. (But newer Linux kernel supports opt-in case-insensitive feature)
For macOS, HFS/HFS+ defaults to case-insensitive if operating system is installed before 10.3. Otherwise it's case-sensitive. For APS, it's case-sensitive.
For Windows, NTFS is actually case sensitive internally, but defaults to case-insensitive in user space.

If you really want to incorporate those tests, you need conditional compilations based on operating system and whehther the filesystem is case sensitive.

My opinion is these cases are too complicated to handle. Just remove those tests.

lebensterben · 2021-04-29T23:19:32Z

lychee-lib/src/types/response.rs

            Status::Timeout(Some(code)) => format!(" [{}]", code),
            Status::Error(e) => format!(" ({})", e),
-            _ => "".to_owned(),
+            _ => "".into(),


No.
When converting a &str to String, I deliberately use .to_owned() to indicate the memory allocation.

This doesn't allocate; I thought about using String::new() but decided that this is clearer.

The whole code base consistently use str::to_owned() for this type of conversion.
When you refactor you need to take the overall coding style and convention into account.

Sure, I'll revert it

lebensterben · 2021-04-29T23:20:06Z

lychee-lib/src/collector.rs

-    } else {
-        None
-    };
+    let base_url = base_url


Again, not necessarily more readable.

I disagree but again, I guess it's a matter of opinion if you think the functional or the imperative style is more readable

soruh · 2021-04-30T00:20:46Z

I will be reverting some of the changes of the original rebase (at least the .not()) and will see about the style related ones once there is a consensus on them.

Regarding the lowercase file extension handling I do think it makes sense to do but can remove it if again if deemed unnecessary.

P.S. @lebensterben I would like to inform you that the way you have reviewed my code has honestly been a very bad experience; This had been the only time that I did not look forward to reading a review of my code.
I personally don't mind that much but I hope you don't always do this as I can't imagine it encourages new contributors to continue contributing (it certainly hasn't for me).

P.P.S I'm pretty tired though which may be affecting my irritability, in that regard. I'll review the rest tomorrow.

lebensterben · 2021-04-30T00:34:51Z

FYI when I refactor, there are a few things taken into account

Does the refactor make it more performant?
Does the refactor make it more readable?
Does the refactor conform to idiomatic Rust coding styles and the overall style of this code base.

Also, I always prefer to make things more explicit. Without writing type signatures everywhere, a way to achieve this to use more specific function/methods.
E.g.

For &str to String, use to_owned instead of into or to_string.
For general conversion of T to U, use U::from(T) instead of T.into().

soruh · 2021-04-30T13:18:38Z

I agree with the first part and usually do the same.
In this case the PR was mostly intended to fix clippy::pedantic warnings (without being fully aware that your PR already did most of that), but while I was at it I added some things that I would do in my code intentionally in a separate commit so that they are easy to remove if not wanted.
Since due to the rebase the PR has become basically only the latter part I agree that it not a very idiomatic refactor.
This version is basically just the code after rebasing it, I'll look into making it more idomatic if I find the time.

mre · 2021-09-04T01:01:23Z

@soruh are you still planning to work on this or shall we close the PR and open a new one if time allows?

soruh · 2021-09-12T19:11:21Z

Ah yeah sorry, I think I'll just close this (don't really have time to work on this at the moment).

mre reviewed Apr 13, 2021

View reviewed changes

src/extract.rs Outdated Show resolved Hide resolved

mre reviewed Apr 13, 2021

View reviewed changes

src/bin/lychee/main.rs Show resolved Hide resolved

mre reviewed Apr 13, 2021

View reviewed changes

lebensterben mentioned this pull request Apr 14, 2021

refactor #208

Merged

soruh added 5 commits April 29, 2021 21:49

fixed clippy complaints (ignored some unreasonable ones)

aa9da92

more style refactors

ad6d758

removed use of or-patterns and made extension check case insensitive

913e566

fix commments magled by rustfmt; make extension checking in FileType:…

2310b02

…:from lowercase

post rebase cleanup

40cd387

soruh force-pushed the clippy-pendantic branch from ed8bed5 to 40cd387 Compare April 29, 2021 21:15

change TODO comment to REVIEW

24a49e0

lebensterben suggested changes Apr 29, 2021

View reviewed changes

mre added the question Further information is requested label Sep 4, 2021

soruh closed this Sep 12, 2021

Uh oh!

Conversation

soruh commented Apr 12, 2021

Uh oh!

soruh commented Apr 12, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mre commented Apr 13, 2021

Uh oh!

soruh commented Apr 13, 2021

Uh oh!

lebensterben commented Apr 13, 2021

Uh oh!

soruh commented Apr 13, 2021

Uh oh!

lebensterben commented Apr 13, 2021

Uh oh!

mre commented Apr 22, 2021

Uh oh!

soruh commented Apr 22, 2021

Uh oh!

mre commented Apr 22, 2021

Uh oh!

soruh commented Apr 29, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soruh Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soruh Apr 30, 2021 •

edited

Loading

soruh commented Apr 30, 2021 •

edited

Loading

soruh commented Sep 12, 2021 •

edited

Loading