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.

Sign up for GitHub

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

Jump to bottom

Add default display format option to config file #1050

Merged
merged 4 commits into from Oct 17, 2020
Merged

Add default display format option to config file #1050

merged 4 commits into from Oct 17, 2020

Conversation

joaquingx
Copy link
Contributor

@joaquingx joaquingx commented Oct 6, 2020 •
edited

  • What is this new code intended to do? Add display_format to config.
  • Are there any related issues? Configure default display format in config #1014
  • What is the motivation for this change? Use e.g. --format md everytime can be tedious.
  • What is an example of usage, or changes to config files? (if applicable)
    Change display_format on config.yml to any of the implemented formatters.

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • I have tested this code locally.
  • I have checked to ensure there aren't other open pull requests
    for the same issue.
  • I have written new tests for these changes, as needed.
  • All tests pass.

Ready to review @wren

@joaquingx joaquingx changed the title [WIP] Add display format option to config file. Add display format option to config file. Oct 8, 2020
wren
wren requested changes Oct 10, 2020
View reviewed changes
Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

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

This looks really good! Tests and docs included, even. Thank you!

Just one small change, and we'll be ready to go.

jrnl/jrnl.py Outdated
# Default display mode
print(journal.pprint())
# Display according display_format config option
config_selected = kwargs["config"].get("display_format")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of nesting this conditional in the parent, can you please move it to the top level with the other elifs?

Copy link
Contributor Author

@joaquingx joaquingx Oct 11, 2020 •
edited

Choose a reason for hiding this comment

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

Hey, thanks, that would be better 👍. Done.

@joaquingx joaquingx requested a review from wren October 14, 2020 13:49
wren
wren previously approved these changes Oct 17, 2020
View reviewed changes
Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

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

Thank you! 🎖️

@wren wren merged commit 6824ca9 into jrnl-org:develop Oct 17, 2020
wren pushed a commit that referenced this pull request Oct 17, 2020
@joaquingx
Add display format option to config file. (#1050)
84f6ed8 
* Add display format option to config file.
* Add tests.
* Fix `black` error.
* Change nested if to top level.
wren pushed a commit that referenced this pull request Oct 17, 2020
@joaquingx @wren
Add display format option to config file. (#1050)
9927264 
* Add display format option to config file.
* Add tests.
* Fix `black` error.
* Change nested if to top level.
wren pushed a commit that referenced this pull request Oct 17, 2020
@joaquingx @wren
Add display format option to config file. (#1050)
0f77d89 
* Add display format option to config file.
* Add tests.
* Fix `black` error.
* Change nested if to top level.
wren pushed a commit that referenced this pull request Oct 18, 2020
@joaquingx @wren
Add display format option to config file. (#1050)
8c2de0e 
* Add display format option to config file.
* Add tests.
* Fix `black` error.
* Change nested if to top level.
@wren wren added the enhancement New feature or request label Oct 18, 2020
@wren wren changed the title Add display format option to config file. Add default display format option to config file Oct 18, 2020
@joaquingx joaquingx deleted the add_display_format_option branch November 9, 2020 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wren wren wren left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@joaquingx @wren