Skip to content

doctor unexpected keys#997

Draft
pmiam wants to merge 8 commits into
papis:mainfrom
pmiam:pm/doctor-unexpected-keys
Draft

doctor unexpected keys#997
pmiam wants to merge 8 commits into
papis:mainfrom
pmiam:pm/doctor-unexpected-keys

Conversation

@pmiam

@pmiam pmiam commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

I wanted a compliment to the required keys check that would help me eliminate pointless metadata from my entries destined for bibtex export.

With this check comes some improvements to the doctor's diagnoses. Namely clearer and more actionable descriptions facilitated by a no dependency TUI list printing function.

As part of the implementation the fixer function edits the user's config file so that may be objectionable.

@alexfikl alexfikl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know this is still a work-in-progress draft, but I left a few comments about the bigger picture to talk about.

Comment thread papis/commands/doctor.py
Comment on lines +497 to +499
msg=f"Document does not define type as one of:\n{
papis.tui.utils.string_grid(sorted(bibtex_types))
}",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is a good idea, since it seems very verbose.

There's about 50 entries in bibtex_types that would be printed for every error. That's ok if you have 2-3 errors, but if you have more than a dozen it just seems like it will print a lot of text. Did you try it out? How does it look?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In general, I agree that we could do a lot better with the error reporting in papis doctor. Should probably try to copy ruff or some newer tools there (e.g line numbers, error codes, better suggestions), but that's quite a bit of work..

Comment thread papis/commands/doctor.py
Comment on lines +641 to +644
BIBLATEX_EXPECTED_KEYS_IGNORED = {
"papis_id", "author_list", "editor_list", "citations", "doc_url",
"files", "ref", "time-added", "type", "tags"
} | frozenset(papis.config.getlist("bibtex-ignore-keys"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe just add most of these directly to bibtex.bibtex_ignore_keys?

Comment thread papis/commands/doctor.py
Comment on lines +666 to +682
config_file = papis.config.get_config_file()
config = papis.config.get_configuration()
lib = papis.config.get_lib_name()
logger.info("[FIX] Adding '%s' to %s.bibtex-ignore-keys", key, lib)

if os.path.exists(config_file):
logger.warning("file contents (whitespace, comments) may change")
if not papis.tui.utils.confirm("Proceed with overwrite?"):
raise Exception("fix canceled by user")

ignores = papis.config.getlist("bibtex-ignore-keys")
ignores.append(key)
config[lib]["bibtex-ignore-keys"] = str(ignores)

with open(config_file, "w") as configfile:
config.write(configfile)
logger.info("Configuration file saved at '%s'.", config_file)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems very convoluted and unsafe for an automated fix. I would recommend just extending the error message with a suggestion e.g. add '{key}' to 'bibtex-ignore-keys' in your configuration file to silence this error?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One thing we could do here, is extend the --edit functionality to allow opening the configuration file instead if the check requests it?

@alexfikl

Copy link
Copy Markdown
Collaborator

@pmiam Still interested in getting something like this in? 😁

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