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

Fix title splitting logic to account for both newlines and periods #958

Merged
merged 11 commits into from
May 27, 2020

Conversation

eshrh
Copy link
Contributor

@eshrh eshrh commented May 19, 2020

This PR changes the logic for separating the title from the rest of the entry. This is intended to fix #945.

It's important to note that we can't just remove all sentence terminal checking, because then normal command-entry breaks. When a user enters an entry at the command line, they obviously can't naturally newline, so we still have to break the sentence by terminals. However, if there is a newline, then we can infer that that was where the user truly wanted the title to end because they were writing in a full text editor.

Because of this, it is impossible to fully fix #945, but it is possible to fix it if there is a newline character following the sentence.

Here are some cases:

User:
jrnl I did it! I won the competition. I am so happy
Result:

2020-05-19 16:15 I did it!
| I won the competition

This is no different from earlier

However, if the user in a text editor writes:
I did it! I won the competition.
I am so happy
The result is now

2020-05-19 16:17 I did it! I won the competition.
| I am so happy

This is different

This fixes #945 without messing up anything else.

Checklist

  • The code change is tested and works locally.
  • Tests pass. Your PR cannot be merged unless tests pass. --
    poetry run behave
  • The code passes linting via
    black (consistent code styling). --
    poetry run black --check . --verbose --diff
  • The code passes linting via pyflakes
    (logically errors and unused imports). -- poetry run pyflakes jrnl features
  • There is no commented out code in this PR.
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open
    Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like
    us to include them?
  • Have you written new tests for your core changes, as applicable?

@eshrh
Copy link
Contributor Author

eshrh commented May 19, 2020

I lost track of the correct order of "",text when i was switching it around. aeafcd3 was supposed to fix this but it looks like it just made the mistake...

@wren
Copy link
Member

wren commented May 22, 2020

I think this is a good compromise for the title splitting. Thank you!

Can you please add some tests for the changed functionality?

@eshrh
Copy link
Contributor Author

eshrh commented May 24, 2020

I added one test for multi-line entries with punctuation and one test for single-line entries with punctuation.

@wren wren changed the title Fix title splitting logic Fix title splitting logic to account for both newlines and periods May 27, 2020
@wren wren added the bug Something isn't working label May 27, 2020
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 2311781 into jrnl-org:develop May 27, 2020
@eshrh eshrh deleted the period-title-bug branch May 27, 2020 21:24
wren pushed a commit that referenced this pull request Jul 25, 2020
)

* remove period parsing in title
* fix title splitter
* revert title-body switch
* keep both splitting types
* make black happy
* make it lstrip not strip
* fix title-body order for the last time
* make black happy again
* added test
* second test for single line entry with punctuation
* delete extra blank lines
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.

No way to have "." in the middle of a title
2 participants