-
Notifications
You must be signed in to change notification settings - Fork 98
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
config: add support for formatted keys #711
base: main
Are you sure you want to change the base?
Conversation
da1df22
to
549782e
Compare
I just tried your fork because I need this feature, but I somehow can't start the
But I get the I guess there is some code upper-casing the |
@hermlon Can confirm that this doesn't work as expected. I'll look into it! |
26187fe
to
03e8cdc
Compare
@hermlon Turns out my case was just a config error. I added a test for the case you mentioned (I think), i.e. setting a Can you give more details about your setup? Do other things work, e.g. does setting EDIT: Gah, nevermind, that was another issue with my setup. Yes, your format didn't work because we do apparently capitalize things by default here: Lines 411 to 416 in c1a9d91
That doesn't sound like a good idea, so we can make a PR to revert it. Thanks for pointing that out and sorry for the confusion! |
03e8cdc
to
49eb3cb
Compare
2dce1ee
to
126825c
Compare
I tried the commit 126825c; it works well, no abnormalities found yet. 👏 One thing is that something like |
Awesome! Thank for you testing!
Hm, it shouldn't be too hard to implement. I'll look into it! There are a few more places that take values that should be formatted from the command line, so we need some nice solution for them. EDIT: I've looked a bit at this and there's two issues here:
instead, but not sure that's very nice.. |
126825c
to
5f73531
Compare
@OopsYao The latest version should also support |
9bb41cb
to
c7e418c
Compare
025a2d2
to
f273de8
Compare
2cede41
to
96264fd
Compare
f82002b
to
ec2443a
Compare
20465d7
to
21c06fd
Compare
21c06fd
to
536b7c1
Compare
I have been trying this feature for a couple of weeks and I really like it! I think the Jinja2 formatting is a great addition, and the formatted keys give a lot of flexibility. While the config is very compact, I find it unusual. Maybe I would've expected:
Thinking about it, this all adds complexity, while settling for Jinja2 as default and only formatter would simplify the code. The default Python formatter is simpler, but if we have a well-known syntax, Jinja2, which is more powerful, could that be the only option? |
@kiike Thank you for trying this out! ❤️
Why is this better? I find it a bit repetitive for multiple options, e.g.
and annoyingly allows these to be set apart, e.g.
I'm not sure I agree with this. First, the jinja2 formatter has been available for a long time and you can already switch to it if it feels more natural. The main issue with that was just that all the default values in Papis are defined for the Python formatter, so it's a bit of a drag to redefine all of them. I also don't think removing the plugins infrastructure for the formatters and having the jinja2 one as the only implementation is likely to happen. At least I have some custom formatters used to hook into formatting references and file names that would be quite annoying to do otherwise.. |
You're right. It gets tedious easily.
Yes. Having to define manually all of them from scratch is quite a drag. Plus it's not straightforward to see the changes without being put off by all the broken strings, or exceptions, and errors and warnings.
Makes sense. Especially in your case, with the custom formatters. Thank you for explaining this! |
Yeah, not quite sure how to solve that in a nice way. That was mostly what this is trying to work around: instead of rewriting all the config settings using e.g. jinja2, you can just rewrite the ones where you need a bit more power. There may be something to be said about switching to jinja2 as the default though. Especially if this goes in, we can probably do that in a backwards compatible way (or at least will less pain). |
1abfd9e
to
d8b2ea1
Compare
d8b2ea1
to
7d0406c
Compare
7d0406c
to
6b82e91
Compare
dbc9e46
to
ac7f3a8
Compare
ef2fc99
to
48581ee
Compare
48581ee
to
1fbafc6
Compare
This adds support for entries like these in the configuration file (see discussion in #662):
where the first setting uses the default formatter (whatever
formatter
is set to) and the second setting overwrites the default and forces thejinja2
formatter. In general,key[.formatter]
is how this looks like.For this, it adds some functionality:
papis.config.getformattedstring
: this looks thorough the config file for any version ofkey.[formatter]
and returns the last one it finds.papis.strings.FormattedString
: a class that's just a(formatter, string)
tuple and gets passed topapis.format.format
to allow picking the formatter for each individual string. This is mostly inspired byprompt_toolkit.FormattedText
.papis.cli.FormattedStringParamType
: aclick.ParamType
that automatically converts command-line parameters toFormattedString
. It also shows a nice text--file-name FORMATTED-TEXT
in the command line to mark entries that can be format strings.TODO:
papis.commands.add::run
works correctly with formatted strings. Need to add some tests for that.Fixes #662.