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

Fixed error related to display_format in config file for some values #1495

Merged
merged 6 commits into from
Jun 4, 2022

Conversation

apainintheneck
Copy link
Contributor

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • 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.

Basically what was happening was that using display_format: pretty or display_format: short in your config file would cause an error. The reason for this was that even though those arguments were handled correctly they were sourced from the config file after those if statements had already been skipped over.

Now _display_search_results tries to source the display_format value from the config file if the export arg is empty before displaying search results. This is a small change so I didn't include any tests. Let me know if you think they are necessary and I'd be happy to add them.

Fixes #1263.

@apainintheneck apainintheneck marked this pull request as draft June 4, 2022 18:12
@apainintheneck
Copy link
Contributor Author

Actually, it looks like this clobbers the --tags flag so I'll take a longer look at it.

Now _display_search_results tries to source the export arg from the
config file before dispaying search results.
@apainintheneck apainintheneck marked this pull request as ready for review June 4, 2022 18:39
@apainintheneck
Copy link
Contributor Author

Okay, I had to change it to evaluate --tags before --format. Also, there was a weird thing that showed up in the unit tests where mock_args.tags was evaluating to true so I changed that as well. The funny thing is that that test isn't even testing for tags so I'm not sure why that happened.

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.

Looks good! Just updated the unit test to fix the tags issue you were running into.

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

Looks good! I just added a BDD test in case of regressions.

@micahellison micahellison added the bug Something isn't working label Jun 4, 2022
@wren wren changed the title Fixed error related to display_format in config file Fixed error related to display_format in config file for some values Jun 4, 2022
@wren wren merged commit c043f50 into jrnl-org:develop Jun 4, 2022
@apainintheneck apainintheneck deleted the fix-display-format branch June 28, 2022 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

display_format: pretty and display_format: short lead to crash
3 participants