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

config: add support for formatted keys #711

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Nov 24, 2023

This adds support for entries like these in the configuration file (see discussion in #662):

[settings]
add-file-name = {doc[year]}-{doc[author]}-{doc[title]}
ref-format.jinja2 = {{ doc.author_list|slice(3)|join("", attribute="family") }}{{ doc.year }}

where the first setting uses the default formatter (whatever formatter is set to) and the second setting overwrites the default and forces the jinja2 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 of key.[formatter] and returns the last one it finds.
  • papis.strings.FormattedString: a class that's just a (formatter, string) tuple and gets passed to papis.format.format to allow picking the formatter for each individual string. This is mostly inspired by prompt_toolkit.FormattedText.
  • papis.cli.FormattedStringParamType: a click.ParamType that automatically converts command-line parameters to FormattedString. It also shows a nice text --file-name FORMATTED-TEXT in the command line to mark entries that can be format strings.
  • Update the rest of the code to use the above where appropriate!

TODO:

  • Need to check what other keys should use formatted strings.
  • Not sure the folder name construction in papis.commands.add::run works correctly with formatted strings. Need to add some tests for that.
  • Probably needs some more testing to make sure nothing is broken. (Currently using this daily, so hopefully will catch some things)

Fixes #662.

@hermlon
Copy link

hermlon commented Nov 25, 2023

I just tried your fork because I need this feature, but I somehow can't start the ref-format with a lowercase letter. I'm using this Jinja format string:

ref-format.jinja2 = {{ doc.author_list[0]['family']|lower }}{{ doc.year }}

But I get the ref: Sanderson12

I guess there is some code upper-casing the ref-format again, since the Jinja2 documentation clearly states that the lower filter lowers all the characters.

@alexfikl
Copy link
Collaborator Author

alexfikl commented Nov 26, 2023

I just tried your fork because I need this feature, but I somehow can't start the ref-format with a lowercase letter. I'm using this Jinja format string:

ref-format.jinja2 = {{ doc.author_list[0]['family']|lower }}{{ doc.year }}

@hermlon Can confirm that this doesn't work as expected. I'll look into it!
Thanks a lot for testing ❤️ this touches a lot of parts, so needs quite a bit more testing besides the unit tests..

@alexfikl alexfikl force-pushed the formatted-strings branch 2 times, most recently from 26187fe to 03e8cdc Compare November 27, 2023 13:17
@alexfikl
Copy link
Collaborator Author

alexfikl commented Nov 27, 2023

@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 ref-format.jinja2, and it seems to work as expected.

Can you give more details about your setup? Do other things work, e.g. does setting ref-format.jinja2 = {{ doc.year }} give the expected change in the ref? Can you also set ref-format in the config to see if it's picking that one by mistake?

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:

papis/papis/bibtex.py

Lines 411 to 416 in c1a9d91

return string.capwords(str(slugify.slugify(
ref,
lowercase=False,
word_boundary=False,
separator=" ",
regex_pattern=allowed_characters))).replace(" ", "")

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!

tests/test_format.py Outdated Show resolved Hide resolved
@alexfikl alexfikl marked this pull request as ready for review December 4, 2023 12:47
@alexfikl alexfikl force-pushed the formatted-strings branch 4 times, most recently from 2dce1ee to 126825c Compare December 12, 2023 07:10
@OopsYao
Copy link

OopsYao commented Dec 15, 2023

I just tried your fork because I need this feature, but I somehow can't start the ref-format with a lowercase letter. I'm using this Jinja format string:

ref-format.jinja2 = {{ doc.author_list[0]['family']|lower }}{{ doc.year }}

But I get the ref: Sanderson12

I guess there is some code upper-casing the ref-format again, since the Jinja2 documentation clearly states that the lower filter lowers all the characters.

I tried the commit 126825c; it works well, no abnormalities found yet. 👏 One thing is that something like papis update --set ref.jinja2 [ref-format] is not supported currently. Don't know if this needs extra heavy work.

@alexfikl
Copy link
Collaborator Author

alexfikl commented Dec 15, 2023

I tried the commit 126825c; it works well, no abnormalities found yet. 👏

Awesome! Thank for you testing!

One thing is that something like papis update --set ref.jinja2 [ref-format] is not supported currently. Don't know if this needs extra heavy work.

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:

  • For the --set key value option that a few of the commands take, we can just use key[.formatter] and select the formatter that way. This shouldn't be hard to implement.
  • Other arguments, e.g. papis add --file-name [FORMAT] ..., don't really have a way to specify the formatter. I'm not sure what a good way to do that would be. We can always recommend using
papis --set add-file-name.jinja2 [FORMAT] add ...

instead, but not sure that's very nice..

@alexfikl
Copy link
Collaborator Author

@OopsYao The latest version should also support papis update --set ref.jinja2 [format]. Let me know if you try it and something doesn't work (I just very briefly tested it)!

@alexfikl alexfikl force-pushed the formatted-strings branch 2 times, most recently from 9bb41cb to c7e418c Compare December 16, 2023 18:47
tests/commands/test_add.py Fixed Show fixed Hide fixed
@alexfikl alexfikl force-pushed the formatted-strings branch 2 times, most recently from 72304bb to 025a2d2 Compare January 29, 2024 16:48
papis/defaults.py Fixed Show fixed Hide fixed
@alexfikl alexfikl force-pushed the formatted-strings branch 3 times, most recently from 2cede41 to 96264fd Compare February 15, 2024 13:02
@alexfikl alexfikl force-pushed the formatted-strings branch 2 times, most recently from f82002b to ec2443a Compare February 23, 2024 07:26
@alexfikl alexfikl force-pushed the formatted-strings branch 3 times, most recently from 20465d7 to 21c06fd Compare March 24, 2024 12:39
@kiike
Copy link
Contributor

kiike commented Apr 10, 2024

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:

add-folder.format = {{doc.author}}
add-folder.formatter = jinja2

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?

@alexfikl
Copy link
Collaborator Author

alexfikl commented Apr 10, 2024

I have been trying this feature for a couple of weeks and I really like it!

@kiike Thank you for trying this out! ❤️

While the config is very compact, I find it unusual. Maybe I would've expected:

add-folder.format = {{doc.author}}
add-folder.formatter = jinja2

Why is this better? I find it a bit repetitive for multiple options, e.g.

header-format.format = ...
header-format.formatter = jinja2
add-file-name.format = ...
add-file-name.formatter = jinja2
add-folder.format = ...
add-folder.formatter = jinja2

and annoyingly allows these to be set apart, e.g.

header-format.format = ...
picktool = ...
opentool = ...
add-confirm = ...
header-format.formatter = jinja2

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?

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..

@kiike
Copy link
Contributor

kiike commented Apr 10, 2024

I have been trying this feature for a couple of weeks and I really like it!

@kiike Thank you for trying this out! ❤️

While the config is very compact, I find it unusual. Maybe I would've expected:

add-folder.format = {{doc.author}}
add-folder.formatter = jinja2

Why is this better? I find it a bit repetitive for multiple options, e.g.

header-format.format = ...
header-format.formatter = jinja2
add-file-name.format = ...
add-file-name.formatter = jinja2
add-folder.format = ...
add-folder.formatter = jinja2

You're right. It gets tedious easily.

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.

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.

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..

Makes sense. Especially in your case, with the custom formatters.

Thank you for explaining this!

@alexfikl
Copy link
Collaborator Author

alexfikl commented Apr 10, 2024

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.

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).

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.

Formatter specific defaults
4 participants