More refactor by lebensterben · Pull Request #222 · lycheeverse/lychee

lebensterben · 2021-04-13T21:25:36Z

Major changes in lychee-lib::filter module:
- Fields in Excludes except the RegexSet is now moved to Filter.
- Filter contains Option<Excludes> and Option<Includes>, which are
  wrapper struct of RegexSet instead of Option<RegexSet>. As a result
  the code now looks cleaner.
- Factored out some filtering logics to dedicated functions.
  - It's possible to write tests for those functions in addition to tests
    for the Filter struct.
- Added docs to Filter::is_excluded and reorgnized the code.
placed derive_builder by typed_builder:
- The internal interface very ugly, as admitted by the author, but we no
  longer have nested Options like before.
- As a result, the Client building is much easier to read.
- Main benefit of typed_builder is, the arguments feeded to builder is
  checked at compile time instead of run-time.
Fixed a bug in lychee::tests::usage and lychee-lib::stats::test.
- Now it will clear environment variable which would otherwise cause an
  issue if GITHUB_TOKEN is set.
Updated dependencies.

lebensterben · 2021-04-13T21:26:03Z

Let's merge this after #208 and I'll combine rebase it.

mre · 2021-04-14T08:50:23Z

lychee-lib/src/filter/excludes.rs

-use crate::Uri;

+// TODO: renaming
+// TODO: should just be added to the Excludes set.


That's a great idea! We could call it default_excludes or something

In a second thought, this is doable but will makes the filtering messy.

Because when only includes are provided without excludes, we require URLs to be explicitly included.

If we add false positives as default excludes, this mechanism need to be changed.

So the updated code removed these TODOs.

lychee-lib/src/filter/excludes.rs

lychee-lib/src/filter/mod.rs

mre · 2021-04-14T08:58:31Z

lychee-lib/src/filter/mod.rs

+    ///
+    /// 1. If any of the following conditions is met, the URI is excluded:
+    ///   - If it's a mail address and it's configured to ignore mail addresses.
+    ///   - If the IP address belongs to a type that is configured to exclude.


is configured to exclude -> is configured to be excluded

No, the grammar is correct.

Consider there's work to be done and there's work to do.
Active infinitive focuses on the agent of action.
This is the documentation of the behaviour of a Filter. So the focus in on Filter.

If you use passive infinitive, the focus transferred to the receiver, which is the Url.

mre · 2021-04-14T09:00:33Z

lychee-lib/src/filter/mod.rs

+
+        if is_false_positive(input)
+        // Exclude well-known false-positives
+        // Performed after checking includes to allow user-overwriddes


overwriddes -> overwrites

This is a common misusage.

See the section title: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

lychee-lib/src/filter/mod.rs

mre · 2021-04-14T09:03:42Z

lychee-lib/src/filter/mod.rs

+            || self.is_excludes_match(input)
+        // If excludes rules matches input, then
+        // *explicitly excluded*
+        {


In the above if conditions, I'd personally also move the comment before the condition. At least I find it easier to read this way, but it's subjective of course. Just wonder if rustfmt would also change the indentation in that case.

if // Exclude well-known false-positives // Performed after checking includes to allow user-overwriddes is_false_positive(input) // Previous checks imply input is not explicitly included, // if excludes rules is empty, then *presumably excluded* || self.is_excludes_empty() // If excludes rules matches input, then // *explicitly excluded* || self.is_excludes_match(input) { return true; }

This is also hard to read

lychee-lib/src/lib.rs

mre · 2021-04-14T09:07:37Z

I love it. Added some comments. Other than that all good.

- Major changes in `lychee-lib::filter` module: - Fields in `Excludes` except the `RegexSet` is now moved to `Filter`. - `Filter` contains `Option<Excludes>` and `Option<Includes>`, which are wrapper struct of `RegexSet` instead of `Option<RegexSet>`. As a result the code now looks cleaner. - Factored out some filtering logics to dedicated functions. - It's possible to write tests for those functions in addition to tests for the `Filter` struct. - Added docs to `Filter::is_excluded` and reorgnized the code. - placed `derive_builder` by `typed_builder`: - The internal interface very ugly, as admitted by the author, but we no longer have nested `Option`s like before. - As a result, the `Client` building is much easier to read. - Main benefit of `typed_builder` is, the arguments feeded to builder is checked at compile time instead of run-time. - Fixed a bug in `lychee::tests::usage` and `lychee-lib::stats::test`. - Now it will clear environment variable which would otherwise cause an issue if `GITHUB_TOKEN` is set. - Updated dependencies.

lebensterben · 2021-04-15T03:50:47Z

The publish error is due to lychee-lib on docs.rs uses derive-builder, so it cannot find the typed-builder dependency.
Again we need to first publish lychee-lib.

Also, this is a breaking change so it would be 0.7.0, or 0.7.0.alpha.

lebensterben · 2021-04-15T16:17:57Z

I will make an attempt to improve the builder interface.
I'll try to put the internal builder in a non-public module. And expose a new builder struct with better API interface.
The downside of typed-builder is that it's less flexible. E.g. you cannot provide custom build() function. But I will try to overcome that limitation.

~~So hold on and don't merge now.~~

Failed.

lebensterben force-pushed the feature-typed-builder branch from 30b4e33 to f1518f5 Compare April 13, 2021 23:59