Skip to content

Add WithSoftWraps option#36

Merged
bwplotka merged 2 commits intoKunde21:masterfrom
saswatamcode:hard-wraps
Aug 10, 2021
Merged

Add WithSoftWraps option#36
bwplotka merged 2 commits intoKunde21:masterfrom
saswatamcode:hard-wraps

Conversation

@saswatamcode
Copy link
Copy Markdown
Contributor

This PR adds WithHardWrap option, which when enabled preserves soft line breaks. Addresses #29.

Feedback is needed on whether this could be done via line widths, in which case some sort of word wrapping is needed and which might lead to further issues as it would override both hard and soft line breaks.

cc: @bwplotka

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Copy link
Copy Markdown
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, some nits only, otherwise LGTM!

main.go Outdated
write = flag.Bool("w", false, "write result to (source) file instead of stdout")
doDiff = flag.Bool("d", false, "display diffs instead of rewriting files")
underlineHeadings = flag.Bool("u", false, "write underline headings instead of hashes for levels 1 and 2")
hardWraps = flag.Bool("h", false, "hard wrap lines even on soft line breaks")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would not add as h - it's usually used for help. Let's use just hard-wraps? ;p

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure! 🙂

_, _ = r.w.Write(spaceChar)
char := spaceChar
if r.mr.hardWraps {
char = newLineChar
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That was.... easy (:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes! 😁

main.go Outdated
opts = append(opts, markdown.WithUnderlineHeadings())
}
if *hardWraps {
opts = append(opts, markdown.WithHardWraps())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think another 3rd option here would be to automatically change it to X chars wide. We can add that later, but wonder if this option would conflict. e.g if someone will use WithHardWraps and WithMaxLineWidth(80)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be added later! I think a check might be needed for the conflict between them. Also, word wrapping needs to be implemented for this in a way(something like this) so that there are no formatting surprises! 🙂

}
}

func WithHardWraps() Option {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we add comment?

Also ... technically we talk about soft wraps, no?

Suggested change
func WithHardWraps() Option {
func WithSoftWraps() Option {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will add a comment and rename it! So everything hardWrap -> softWrap.

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode saswatamcode changed the title Add WithHardWraps option Add WithSoftWraps option Aug 9, 2021
@saswatamcode saswatamcode requested a review from bwplotka August 9, 2021 16:28
Copy link
Copy Markdown
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice! LGTM (:

@bwplotka bwplotka merged commit 727f02f into Kunde21:master Aug 10, 2021
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.

2 participants