Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[WIP] [Console] Pretty word wrapping#30590

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
fchris82 wants to merge17 commits intosymfony:4.2fromfchris82:4.2
Closed

Conversation

@fchris82
Copy link
Contributor

@fchris82fchris82 commentedMar 17, 2019
edited
Loading

QA
Branch?4.2
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#30519
LicenseMIT
Doc PRsymfony/symfony-docs#11190

This new feature was born from a bugfix. This feature add a PrettyWordWrapping class that you can use in console, when you want to wrap the text.

@nicolas-grekasnicolas-grekas added this to thenext milestoneMar 17, 2019
@fchris82fchris82 marked this pull request as ready for reviewMarch 19, 2019 16:10
@fchris82
Copy link
ContributorAuthor

@ro0NL Let's discuss! :) I fully refactored the code, I made a helper from it, and integrated intoTable andOutputFormetter. I changed a little bit your solution, and separated "wrapping" from "formatter" class because it would have been too complex in it.

*
* @return string
*/
publicfunctionwordwrap(string$message,int$width,int$cutOption =null):string;
Copy link
Contributor

@ro0NLro0NLMar 19, 2019
edited
Loading

Choose a reason for hiding this comment

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

This is new feature isnt it? To wrap a raw string with tags (unformatted still) ...

Do we really need it? As such it's a BC break, and it's the exact same reason we added a newWrappableOutputFormatterInterface in 4.2.

New features target master and thus would imply another interface. I think we should avoid that and solve the issue informatAndWrap().

Which takes me to my 2nd point; this PR will parse the tags twice IIUC. My main motivation for "formatAndWrap" was to be able to handle it at once. I think we should patch it instead, relying on a helper if really needed for clarity.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I was thinking about it also. I saw 3 ways:

  1. I add the CUT option parameter to theformatAndWrap() --> this causes BC break
  2. I create a new wrap function that only can wrap --> it doesn't cause BC break
  3. I leave the CUT option parameter --> it doesn't cause BC break, but it causes inflexible wrapping function

Maybe there is a 4. option: I removewordwrap() (maybe theformatAndWrap() too), and if somebody want to wrap then call like here:

// Without wrapping$output->getFormatter()->format($text);// With wrapping$output->getFormatter()->format(    PrettyWordWrappingHelper::wordwrap($text,120));

It would be clearer, but it supersede yourWrappableOutputFormatterInterface because wrapping and formatting are really and fully separated (and helps to programmers changing it a custom wrapper if they want) I would prefer this.

this PR will parse the tags twice

Yes, it is true. This is a side effect, which I think is necessary. Formatting (styling?) and wrapping can enough complex separately, that is why I think we should handle it separately. It causes more maintenance and extendable code. And this will be used in command line, the performance isn't so important. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

option 5) Your "With wrapping" example to me equalsformatter->formatAndWrap($text, 120) already. That's API in 4.2 we can fix, maybe even with an@internal WordwrapHelper, or public for 4.3/master.

option 6) deprecateWrappableOutputFormatterInterface in 4.3 and add the "best" API directly to OutputFormatterInterface using#28902 (which wasnt possible back then)

In general i think the wordwrap helper should be an implementation detail of the output formatter. But we need some consensus first to move forward. Given a lot of work is done here already, perhaps we should finish it :) The bug is real.

this PR will parse the tags twice

Yes, it is true. This is a side effect, which I think is necessary.

You know best :) so far i thought it seemed reasonably possible ... but it's also still flawed currently 😅 I need to look closer at this new approach to see how it differs.

Copy link
ContributorAuthor

@fchris82fchris82Mar 22, 2019
edited
Loading

Choose a reason for hiding this comment

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

... I'm thinking about it ...

}

$lines =array_merge($lines,explode(PHP_EOL,wordwrap($message,$this->lineLength -$prefixLength -$indentLength,PHP_EOL,true)));
$lines =array_merge($lines,explode(PHP_EOL, PrettyWordWrapperHelper::wrap(
Copy link
Contributor

Choose a reason for hiding this comment

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

basically i started my patch by invokingformatAndWrap() here, which conceptually works.

Copy link
Contributor

@ro0NLro0NL left a comment
edited
Loading

Choose a reason for hiding this comment

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

Changed fixtures look sensible 👍 , but the amount of code introduced is hard to grasp. E.g. do we really need all theCUT_ options (now)?

This seems like a lot of new features combined, which doesnt make them wrong, but i'd prefer smaller PRs personally.

At this point, aren't we better of to rewrite the tag parsing as a whole? I.e. using a (real) lexer.

@javiereguiluz
Copy link
Member

I like this feature, but I'm concerned about the naming:

  • I'd removePretty fromPrettyWordWrapperHelper because this feature is always called "Word Wrapping" ... so that should be enough. So,PrettyWordWrapperHelper ->WordWrapperHelper
  • I'd removeLONG fromCUT_LONG_WORDS because words can be short and still be cut or not. So,CUT_LONG_WORDS ->CUT_WORDS
  • I don't understand what these options do:CUT_FILL_UP_MISSING,CUT_ALL.

@fchris82
Copy link
ContributorAuthor

About CUT options/flags

@ro0NL and@javiereguiluz Before I reply for you one by one, I would like to clarify the importance of CUT options. I started this feature because of my existing other program, where I had to print some well formatted and colorized help/description texts, which were ugly with the existing output formatting solution. These are longer texts, with some URLs.
First I wrote a custom wrapper half year ago and I used it. My plan was that I will implement this function into Symfony when I have free time. While I was writing the code, I noticed that there are 2 big different situations because ofTable helper. As you can see here:https://github.com/symfony/symfony/pull/30519/files#diff-a3a53b968f8b0f15e54e6eeef311a73aR147 , first there was only one boolean "parameter" instead ofCUT options: $cutLongWords. Exactly it wasn't "my idea", I would have preferred if the implementedwordwrap() function had been similar to phpwordwrap() function:http://php.net/manual/en/function.wordwrap.php . So the$cutLongWords would have been same like the php$cut parameter in nativewordwrap().
But theTable console helper showed me, I didn't consider "very short" lines (<10-20 chars). The base solution isn't enough "responsive" :D I started thinking about other problems too, and because I would want to create a really flexible solution, first I added a second boolean parameter, and then a third, and that was the point when I thought, it would be too much:

publicfunctionwordwrap($string,$witdth,$cutLongWords =true,$cutURLs =false,$fillUpEnd =false, ...){// ...}

There are many other functions where there are lots of configuration options, e.g:

If you have long rows (>50-60 chars) you may not need anyCUT options, but as you can see in the documentation at the CUT examples:https://github.com/symfony/symfony-docs/pull/11190/files#diff-8d05f5767d5a596b67434f76c113bb8dR80 , there are different "good" solutions, and I would like to leave the choice for the programmer, which one they want to use. Eg ($width = 4):

No cut:

Loremipsumdolorsitamet

Cut every words (sit andamet also):

Lorem ipsumdolor sit amet

Cut only the long words, that are longer than$width (sit andamet aren't cut)

Lorem ipsumdolorsitamet

Please look at the documentation too:https://github.com/symfony/symfony-docs/pull/11190/files , there are examples and after that maybe the conception will be clearer.

@fchris82
Copy link
ContributorAuthor

At this point, aren't we better of to rewrite the tag parsing as a whole? I.e. using a (real) lexer.

I was thinking about it also. I started to do a tokenizer/lexer, but I rejected it fast. The "grammar" is "too" simple. There are words and some simple tags. There aren't tag attributes, special characters, placeholders, etc. Yes, a lexer would be nicer, but we don't forget: this is the console block, for commands, what are used by programmers - in most cases -, and not "non-professional" users. We need a wrapper for "help/description/information" texts and for table cells. That's all.

*@return int
*/
publicfunctionformat($message)
publicfunctiongetDefaultWrapCutOption():int
Copy link
Contributor

Choose a reason for hiding this comment

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

not really needed is it

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

A protected property that you can only write? It looks a little bit bad concept. A property what you can only read, it could be justified, but a property without reading option, not really.

Copy link
Contributor

Choose a reason for hiding this comment

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

im skeptical about the setter too :)

setters/getters can live independently IMHO, it's not needed for every property by definition or so. If we dont have reason to add a getter/settter i would avoid it (it introduces extra state).

In this case i'd prefer passing the cut options to formatAndWrap(), i dont see a real reason to control this default behavior.

*
* @see PrettyWordWrapperHelper
*/
publicfunctionsetDefaultWrapCutOption(int$defaultWrapCutOption)
Copy link
Contributor

Choose a reason for hiding this comment

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

same, where do we use it? I tend to prefer having a$cutOptions as 3rd argument offormatAndWrap(), which makes it a clear new feature for 4.3

thrownew \LogicException(sprintf('Setting a maximum column width is only supported when using a "%s" formatter, got "%s".', WrappableOutputFormatterInterface::class,\get_class($this->output->getFormatter())));
}

$this->columnMaxWidths[$columnIndex] =$width;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would use[$width, $cutOption] to make it clear this belongs together.


return$this;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we need all this extra API, i would keep it out at first.

@fchris82
Copy link
ContributorAuthor

Hello@javiereguiluz ,

thank you for your comment.

  • I'd removePretty fromPrettyWordWrapperHelper because this feature is always called "Word Wrapping" ... so that should be enough. So,PrettyWordWrapperHelper ->WordWrapperHelper

OK, it was a good idea, I have changed it :)

  • I'd removeLONG fromCUT_LONG_WORDS because words can be short and still be cut or not. So,CUT_LONG_WORDS ->CUT_WORDS

There exists aCUT_WORDS option, that means: cut every words (row length: 8 chars):

Lorem ipsum dolor sit amet

CUT_LONG_WORDS means: cut words that are longer, then a row, shorter words won't be cut (row length: 8 chars):

Loremipsumdolorsitametthisisaverylongword
  • I don't understand what these options do:CUT_FILL_UP_MISSING,CUT_ALL.

CUT_FILL_UP_MISSING means: every row will be same length. It would be important when the line ha background color:

Default show:

▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧

WithCUT_FILL_UP_MISSING (every line is same length):

▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧▧

The missing characters are spaces, and the background color will "flow to line endings".

@fchris82
Copy link
ContributorAuthor

fchris82 commentedMar 27, 2019
edited
Loading

@ro0NL What if there was a<wrap> format style? I would keep theWordWrapperHelper, sometimes it could be useful, but people could use eg:

// It prints the text in a 8 char length, yellow background box$output->writeln('<wrap=8,fillup;bg=yellow>Lorem <info>ipsum</info> dolor sit amet.</>');

And I would refactor the text "pattern" with a lexer/tokenizer.


So, what I would do:

  • SetWrappableOutputFormatterInterface to deprecated
  • Remove my changes fromOutputFormatter, refactor it whit a lexer
  • Add<wrap> "tag"
  • Refactormax column width inTable (by adding<wrap> tag eg)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@fchris82@javiereguiluz@ro0NL@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp