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

Accept some settings as environment variables. #15

Closed
wants to merge 4 commits into from
Closed

Accept some settings as environment variables. #15

wants to merge 4 commits into from

Conversation

JulienPalard
Copy link

Beware it's my first bits in rust, I don't know what I'm doing.

Context: I'm using bkt many times in the same file and would like for all caches to land in the same cache directory and be reusable for way more than 30s but I don't want to add --cache-dir and --ttl at each line.

More context: I'm writing slides in Markdown, but don't want to copy/paste commands output in my slides (I'll have too much), so I'm using a simple sed trick to execute the commands from the Markdown file, but now it's too slow to render the slides so I want to use bkt to cache commands output, but now it's too verbose so I'd like to configure bkt from my Makefile with some environment variables instead of from the Markdown files...

@@ -169,6 +169,13 @@ Note that the choice of directory can affect `bkt`'s performance: if the cache
is stored under a [`tmpfs`](https://en.wikipedia.org/wiki/Tmpfs) or solid-state
partition it will be significantly faster than caching to a spinning disk.

### Environment Variables
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to document these variables alongside their sibling flags, rather than in a separate section. If you feel this is preferable each variable here should at least link to the relevant section of the readme, and call out which flag it's replacing.

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be better to document these variables alongside their sibling flags

I fear adding this to the main usage section would be overwhelming. Using environment variables instead of command line flags is a niche usage, don't need to bother everyone with this.

@dimo414
Copy link
Owner

dimo414 commented Jun 25, 2022

Thanks a lot, I love that clap makes this PR so easy! Could you add some test coverage as well in cli.rs?

@dimo414
Copy link
Owner

dimo414 commented Jun 25, 2022

Also, please share how you're using bkt in the discussions, it sounds interesting!

@JulienPalard
Copy link
Author

Could you add some test coverage as well in cli.rs?

I sadly don't think I'll have time to look at it this week, I try (and fail) to handle my todo list in a FIFO order, but I often go yak shaving. This PR is clearly yak shaving: I was just writing Python slides initially and BOOM I am learning rust...

I'd love to learn rust, and I'll do it, it looks like a very promising language, but I have to be honest with myself: not between slide 3 and slide 4 ;)

So if you want to take this PR from here and fix what you'd like to see fixed, don't hesitate, just push to my branch.

If you too don't have time, no worries, I'll take a look "later".

@JulienPalard
Copy link
Author

Also, please share how you're using bkt in the discussions, it sounds interesting!

Done :)

@dimo414
Copy link
Owner

dimo414 commented Jun 27, 2022

Sounds good, we'll see who finds time first 😆

dimo414 added a commit that referenced this pull request Jul 8, 2022
variables in addition to flags.

I opted not to add an environment variable for --stale at this time
because I'm not convinced how useful it would be. It's easy enough to
add in the future, though I would prefer a more descriptive name than
BKT_STALE if it's added.

BKT_CACHE_DIR serves a very similar purpose to BKT_TMPDIR which was
added in 77c5d91. I decided to use a separate variable to more directly
mirror the --cache-dir flag. Notably, BKT_TMPDIR is respected by the bkt
library whereas BKT_CACHE_DIR is only evaluated at parse time in the
binary. In the future these variables might also have slightly different
behavior, e.g. bkt might create a different directory heirarchy if
BKT_TMPDIR is set instead of BKT_CACHE_DIR.

Credit to @JulienPalard for the idea in #15.
@dimo414
Copy link
Owner

dimo414 commented Jul 8, 2022

Thanks again for sending this, I've pushed a change that adds BKT_TTL, BKT_SCOPE, and BKT_CACHE_DIR.

@dimo414 dimo414 closed this Jul 8, 2022
@JulienPalard JulienPalard deleted the mdk-env branch July 10, 2022 17:31
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