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

Add WithSoftWraps option #36

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Conversation

saswatamcode
Copy link
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
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
@@ -23,6 +23,7 @@ var (
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
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
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
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
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
@@ -62,6 +63,9 @@ func processFile(filename string, in io.Reader, out io.Writer) error {
if *underlineHeadings {
opts = append(opts, markdown.WithUnderlineHeadings())
}
if *hardWraps {
opts = append(opts, markdown.WithHardWraps())
Copy link
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
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! 🙂

@@ -55,6 +56,12 @@ func WithUnderlineHeadings() Option {
}
}

func WithHardWraps() Option {
Copy link
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
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
Copy link
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.

None yet

2 participants