Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional features #224

Closed

Conversation

lebensterben
Copy link
Member

@lebensterben lebensterben commented Apr 15, 2021

  • Some optional features are unnecessary and are removed.
  • console can pad strings, but it currently needs a patch. And pad
    is no longer needed.
  • Added optional features to lychee
    • config_file enables setting options from a TOML file.
    • json_output enables outputting in JSON format.
    • indicatif enables progress bar.
    • By default they're all enabled.
    • Unsupported CLI options are hidden accordingly.

Next:

  • Replace hubcaps with github-rs to see whether M1 architecture can build lychee.
  • Updated CI to check different combinations of features.

- 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.
- Some optional features are unnecessary
- `console` can pad strings, but it currently needs a patch. And `pad`
  is no longer needed.
- Added preminary support for opt-out `serde` feature. (WIP)
If `serde` is not enabled, `lychee` doesn't support JSON output format.
This maybe useful for non-technical users.
@lebensterben
Copy link
Member Author

lebensterben commented Apr 15, 2021

I will rebase once #222 #225 is merged.

@mre
Copy link
Member

mre commented Apr 15, 2021

Replace hubcaps with github-rs to see whether M1 architecture can build lychee.

github-rs doesn't seem to be actively maintained at this point (last commit in May 2019).
In fact I used github-rs before I moved to hubcaps because it was more actively maintained and had support for async reqwest, which github-rs does not afaik.

@lebensterben
Copy link
Member Author

@mre
But hubcaps doesn't work well on M1.

@mre
Copy link
Member

mre commented Apr 15, 2021

That's true. If we can we should support them with adding that. My feeling is we'll run into other issues with github-rs further down the line. To be honest I don't mind keeping the explicit dependency on ring right now to have M1 support.

@lebensterben
Copy link
Member Author

github-rs will not be the big issue because GitHub API is stable, and we only need very minimal API (Token and Repository).

The true problem is the rust-native-tls it depends on. This doesn't rely on ring but it is not actively developed.

So we can use github-rs for aarch.apple.darwin, and in the long run M1 compatibility should get better.

Also hubcaps is not well-maintained. It uses old dependencies which is a serucity risk.

@lebensterben lebensterben changed the title More refactor Optional features Apr 15, 2021
@mre
Copy link
Member

mre commented Apr 15, 2021

That's fine with me, even though I'd say depending on two outdated crates is more of a security risk than just one. I'd personally just keep ring as an explicit dependency for now and try to get an updated version of hubcaps out, which would help other downstream crates as well.

lebensterben and others added 4 commits April 16, 2021 19:27
- Added `config_file` and `json_output` as features of `lychee`
  - `config_file` enables `lychee` to read options from a `toml` config file.
  - `json_output` enables `lychee` to output in JSON format.
  - These two features are enabled by default. Turning off these features
    will cut off dependency on `serde`. It also turns off `lychee-lib`'s
    dependency on `serde` as well.
- `indicatif` is an optional dependency. It's enabled by default but it's hardly
  useful in CI.
- Refactor:
  - The `lychee::main::run` function is too long. Some of its functionalities
    are factored out to dedicated functions.
  - Notably, `Config` now has a bunch of new methods that parses various fields.
- Exported `lychee_lib::client::Client` as public struct (with private fields).
This reverts commit d2e6a18, reversing
changes made to 98e7d1a.
@lebensterben
Copy link
Member Author

lebensterben commented Apr 17, 2021

There's something wrong with CI.
It's not reading the #[cfg()] attributes.

Testing locally doesn't show any problem.
I actually tested with all combinations of feature flags.

@mre
Copy link
Member

mre commented Apr 22, 2021

Humm... no clue what's going on yet.

@mre
Copy link
Member

mre commented Dec 2, 2021

With #330 merged, can you rebase your changes on top of master @lebensterben?

@mre
Copy link
Member

mre commented Feb 4, 2022

@lebensterben can you rebase this to take a look at the failing #[cfg()] flags on CI again?

@lebensterben
Copy link
Member Author

@mre
it may take a few days.
I've another big repository to maintain.

@lebensterben
Copy link
Member Author

After some review I concluded that it's much easier to restart a PR...
So I'm closing this.

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