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

Initial library support #679

Merged
merged 18 commits into from
Oct 20, 2019
Merged

Initial library support #679

merged 18 commits into from
Oct 20, 2019

Conversation

DrSensor
Copy link
Contributor

@DrSensor DrSensor commented Oct 6, 2019

Fix #423

Project Structure

Cargo has a convention where other executables can be placed in src/bin/*.rs. This PR use that convention which move some src/*.rs that depend on app.rs, clap_app.rs, or clap crate into src/bin/bat. Summary, the src directory became:

$ tree src
src
├── assets.rs
├── bin
│   └── bat
│       ├── app.rs
│       ├── clap_app.rs
│       ├── config.rs
│       └── main.rs
├── controller.rs
├── decorations.rs
├── diff.rs
├── dirs.rs
├── inputfile.rs
├── lib.rs
├── line_range.rs
├── output.rs
├── preprocessor.rs
├── printer.rs
├── style.rs
├── syntax_mapping.rs
├── terminal.rs
└── util.rs

2 directories, 19 files
Build and run
cargo build --bins           # to produce binary executable
cargo run                    # just works fine
cargo doc --no-deps --open   # to build and open generated library docs

@DrSensor DrSensor marked this pull request as ready for review October 6, 2019 08:04
@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2019

Thank you very much for looking into this!

The changes here look great. There are a few things that I would like to know before merging this:

  • How does the current state of the bat-as-a-library API compare with prettyprint? How would a simply example program look like?
  • Following up on the previous point: would it make sense to try and support a similar builder-pattern style as prettyprint and maybe even aim for compatibility? From my point of view, it would be great to merge the two projects back together (/cc @mre). Of course, this does not need to happen in this PR.
  • I saw that you also adapted the deployment scripts and config files (thank you!). Still, I would like to make sure that the deployment process has not been broken by these changes. I know this is quite a lot to ask, but would it be possible for you to set up Travis CI / Appveyor on your fork and try out the deployment with a fake release?

@mre
Copy link

mre commented Oct 6, 2019

From my point of view, it would be great to merge the two projects back together (/cc @mre).

Absolutely! Would happily deprecate prettyprint in favor of bat-as-a-library. 😊

would it make sense to try and support a similar builder-pattern style as prettyprint and maybe even aim for compatibility?

If we could have the builder pattern, that would be great. I think that was quite well received from the few users that integrated prettyprint into their crates. rust-derive-builder is where it's at. If anyone likes to copy the code directly from prettyprint, please go ahead.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2019

@DrSensor Could you please run cargo fmt to fix the CI error?

@sharkdp
Copy link
Owner

sharkdp commented Oct 9, 2019

Thank you very much.

Happy to merge this if we can somehow clarify point 1 and 3 from my comment above

@DrSensor
Copy link
Contributor Author

DrSensor commented Oct 9, 2019

Not yet, I want to add examples/*.rs first before answering the comment.

@DrSensor
Copy link
Contributor Author

How does the current state of the bat-as-a-library API compare with prettyprint? How would a simply example program look like?

Finally done 8caca23
See examples/*.rs

Following up on the previous point: would it make sense to try and support a similar builder-pattern style as prettyprint and maybe even aim for compatibility? From my point of view, it would be great to merge the two projects back together (/cc @mre). Of course, this does not need to happen in this PR.

I agree with this though I still want use the low-level/original API for several use-cases. Maybe put the builder-pattern version in different crate via workspace 🤔

the project structure might be something like this:
.
├── Cargo.toml
├── crates
│   └── builder
│       ├── Cargo.toml
│       └── src
│           └── lib.rs
└── src
    ├── bin
    │   └── bat
    │       └── main.rs
    └── lib.rs

I saw that you also adapted the deployment scripts and config files (thank you!). Still, I would like to make sure that the deployment process has not been broken by these changes. I know this is quite a lot to ask, but would it be possible for you to set up Travis CI / Appveyor on your fork and try out the deployment with a fake release?

Unfortunately I'm not familiar with both CI, especially Appveyor. More specifically about what and where the credentials should be put on. The changes are made because the fixtures (bat src/main.rs README.md) is moved, so I think it will not breaking the automated releases.

If the CI does release on tagging, you can checkout this PR and tag them with a fake version to test things out.

Assuming hub is installed, run

git clone https://github.com/sharkdp/bat
cd bat

hub pr checkout 679
git tag -a v0.0.7 -s -m 'James Bond'
git push --tags

then remove the Draft/Release along with Tag after you have verified it

@DrSensor DrSensor requested a review from sharkdp October 15, 2019 08:05
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates. I only have minor comments/questions regarding the examples.

@sharkdp
Copy link
Owner

sharkdp commented Oct 15, 2019

I agree with this though I still want use the low-level/original API for several use-cases. Maybe put the builder-pattern version in different crate via workspace thinking

We can definitely expose the low-level API, but I think it will be possible to additionally expose a builder-style high-level API. No need for a different crate, hopefully. This can be done in a separate PR.

@sharkdp
Copy link
Owner

sharkdp commented Oct 15, 2019

The deployment test was successful! Thank you for the hint about just tagging a release from the feature branch. Hadn't thought of that.

@DrSensor DrSensor requested a review from sharkdp October 16, 2019 23:23
* Move {controller,output,printer,decorations}.rs into src/bin/

* Add `mod errors` from main.rs
This will limit [[bin]] to *bat* only which will make:
- `cargo run` works without specifying --bin
- prevent `cargo build --bins` to produce multiple binaries (app,clap_app,...)
* Change `mod errors` in lib.rs to public

* Add `fn handle_error` in lib.rs errors module
others:
bin/bat/{controller,decorations,output,printer}.rs
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the updates!

I will probably work a bit on the two example files, but there is nothing preventing this from being merged 👍

@sharkdp sharkdp merged commit 4664fb6 into sharkdp:master Oct 20, 2019
@sharkdp sharkdp mentioned this pull request Oct 20, 2019
3 tasks
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.

bat as a library
3 participants