-
-
Notifications
You must be signed in to change notification settings - Fork 525
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
Add machine readable --list output #1592
Add machine readable --list output #1592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just a couple notes about separation concerns in the review. Also, let us know if you'd like any help with adding tests for this.
jrnl/output.py
Outdated
if format == "json": | ||
import json | ||
|
||
return json.dumps(configuration["journals"]) | ||
elif format == "yaml": | ||
from io import StringIO | ||
|
||
from ruamel.yaml import YAML | ||
|
||
output = StringIO() | ||
YAML().dump(configuration["journals"], output) | ||
return output.getvalue() | ||
else: | ||
result = f"Journals defined in config ({config.get_config_path()})\n" | ||
ml = min(max(len(k) for k in configuration["journals"]), 20) | ||
for journal, cfg in configuration["journals"].items(): | ||
result += " * {:{}} -> {}\n".format( | ||
journal, ml, cfg["journal"] if isinstance(cfg, dict) else cfg | ||
) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the value in the simplicity of this approach, though I would hope to see the same information regardless of what format is used. In particular, the nested configuration dictionary doesn't flatten the journal paths between journals specified using "journalname: path" and "journal: journalname: path". And while the old approach already combined the concerns of data retrieval and data presentation, it would be good to keep those separate if we're expanding the feature.
Instead, could you try retrieving the journal data first (sort of like the code in the else
block does), then sending that data to different export functions based on the file format? Probably best as a dictionary object. That way it's all working with the same data while respecting the separation of concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this with a different function for each output format?
journal_list = {
"config_path" : config.get_config_path(),
"journals" : configuration["journals"],
}
if format == "json":
return journal_list_to_json(journal_list)
elif format == "yaml":
return journal_list_to_yaml(journal_list)
else:
return journal_list_to_stdout(journal_list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, that looks great!
jrnl/output.py
Outdated
YAML().dump(configuration["journals"], output) | ||
return output.getvalue() | ||
else: | ||
result = f"Journals defined in config ({config.get_config_path()})\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include this config path information in the other exports as well? I think config_path
is a good key name for it.
Create three new methods for each journal list format. - JSON, YAML, STDOUT
ab111cd
to
f8ea886
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks great!
Checklist
for the same issue.
This closes #1445.
The following options are now available for the list command. If another format option is chosen that isn't yaml or json, it just returns the default list.