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

FileConventions: fix WrapText function #135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

parhamsaremi
Copy link
Contributor

@parhamsaremi parhamsaremi commented Aug 9, 2023

Ignore codeblocks while splitting the text into paragraphs because there might be a codeblock with multiple paragraphs.

Fixes #117

Add failing test for WrapText function based on [1].

[1] nblockchain#117
@parhamsaremi
Copy link
Contributor Author

@knocte @aarani please review.

@knocte
Copy link
Member

knocte commented Aug 10, 2023

@parhamsaremi my review is the following:

Sometimes we cut corners to get something to work, do some hack here and there. You didn't do this with this code, it seems. However, you went to the other extreme: you overengineered. Look at the diff, it's too complicated for such a simple thing that should just skip wrapping code blocks.

You can write much much simpler code by having a loop that goes over all lines, and as soon as it finds three ticks and an EOL, stop wrapping text, and as soon as it finds it again, resume the text wrapping operation. For such a simple algorithm, you don't need regular expressions at all.

@parhamsaremi parhamsaremi force-pushed the issue-117 branch 5 times, most recently from f3486b8 to 0e1b393 Compare August 15, 2023 13:24
@knocte
Copy link
Member

knocte commented Aug 16, 2023

@aarani please review

@parhamsaremi parhamsaremi force-pushed the issue-117 branch 2 times, most recently from a69af6f to fe739b6 Compare August 16, 2023 11:42
Ignore codeblocks while splitting the text into paragraphs
because there might be a codeblock with multiple paragraphs.

Fixes nblockchain#117
@knocte
Copy link
Member

knocte commented Aug 18, 2023

@aarani good now?

Copy link
Contributor

@aarani aarani left a comment

Choose a reason for hiding this comment

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

LGTM

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.

wrapLatestCommitMsg script failed ignoring a code block
3 participants