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

Keep newlines in the text formatter #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hferreiro
Copy link

Replace whitespace manually and keep the text newlines. Also, keep the
stories ordering.

Replace whitespace manually and keep the text newlines. Also, keep the
stories ordering.
@guijemont
Copy link
Contributor

This is a change of behavior that breaks my personal workflow (and maybe others). I suggest adding a command-line option for this (or at least providing a command-line option to be able to have the old behavior).

@hferreiro
Copy link
Author

Can you share your workflow?

@guijemont
Copy link
Contributor

Ok, after checking, it doesn't breaks it, but it looks less good to me. I run with -f markdown and copy-paste the result into our weekly updates tool, as well as save the result locally. I like the format of having "one line per day", but with your patch, I get linebreaks within day entries/items, which is inconsistent with my archive, though the new linebreaks are ignored by the markdown parser, so it's not as bad as I initially thought.

My point remains: I think this is a matter of preference, and therefore should be an option, otherwise I would have to do manual edits to the report every week to make it look the way I like.

@hferreiro
Copy link
Author

Well, I think that this was clearly a bug in that newlines entered by the user were omitted. But, if keeping the old behaviour is more important, I guess it could be made so that the user can write 2 newlines to enter a real newline. I guess it's up to Martin :-)

@guijemont
Copy link
Contributor

IMHO, having the old behavior under a --ignore-newlines option is fine, and probably preferable to having to write double newlines, which feels awkward to me. Also, if you want a newline to appear in the markdown result, that means you would have to write 4 newlines in phpreport (since markdown ignores simple newlines)

Copy link
Owner

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

I think I'm having a bit of trouble understanding the use case for this. Maybe you could give a quick screenshot or text output of the difference between the old and the new approach.

# Many times people add a lot of duplicate descriptions. Just output one of each.
all_stories = set(all_stories)
# Many times people add a lot of duplicate descriptions. Just output
# one of each, keeping the stories ordering
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: "keeping the stories ordering" should probably read "keeping the stories in the same order."

Copy link
Owner

Choose a reason for hiding this comment

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

BTW, this seems like a separate change. Do you mind opening up a PR just for this? I'd like to commit it apart from preserving the newlines.

initial_indent=indent,
subsequent_indent=indent).strip()
second_column = os.linesep.join(
[textwrap.fill(line,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure I understand what is going on here. Are you wrapping each individual line? Is each line really a paragraph?

@mrobinson
Copy link
Owner

@hferreiro So I've been thinking about this and I think that since everyone wants a slightly different output, the best approach here would be for PHPReport to use a template language to produce the reports. That way people could have their own custom templates or simply tweak the existing ones. Would that work for you?

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.

3 participants