wrap-php multiple lines #437
For somewhat reason the (by default) enabled option wrap-php does not work when multiple statements are following line by line.
The text was updated successfully, but these errors were encountered:
@bfrmtx thanks for the report...
I assume the problem is that the above php will be output as one line...
In checking the code, it seems some time back we removed a conditional line flush, after each php block... I have not had a chance to look deeper into why? When?
But as a test, I applied the following small patch -
While that restores each php statement to a new line, whether you add one or not, I am not sure it is the correct use of
And what about if you write -
Should they be kept as one line, or a newline added? At the time these blocks are output tidy has no memory of whether the user put them on one line, or over many lines... that information is lost...
But have I got the right idea here? Look forward to your further comments...
@bfrmtx, well I explored back into the source to find out exactly when that
Tidy has always been like this... that is never placing successive
I have not yet done a search in all the issues... ie has this come up before... strange if it has not... but, sure, it seems worthy of consideration...
Now I do not write much
With this patch this becomes -
Now while browsers, like tidy, tend to ignore newlines, they will usually add at least a
So while this simple patch, or a similar patch, is going to potentially change things for those that do use
And like I advised, at the time of this
Maybe we do need a way to retain that info, and use it to help decide, and this may assist in other issues, like say #329 - still open...
We do still have one open
So, like I say, seems like a sound new feature, but since tidy has been like this for 15 plus years, will leave it a little time longer for more consideration and comments...
What do other think? Thanks...
Ok, that's fine for me.
I have added a test case (with fake number) for you - in case it becomes a new feature.
@bfrmtx thanks for taking the time and considering this more...
Ok, if we go for a new feature what would be its name?
And there is a README OPTIONS.md, which enumerates the basic four steps of a new option, the last just being its use in the code...
And adding a test regression case is optional, and anyway it is in another repo... note the
But maybe we are getting ahead of ourselves... maybe a new option is not required!
First we need to understand what does the current
The description is simple "... should line wrap text contained within PHP pseudo elements, which look like:
Now it seems in
And when you think about it more, how could tidy wrap php script!
It does not know php syntax, and thus has no idea where line breaks would be allowed in such a script... ergo, it can not wrap any of this inner php text...
While maybe indenting is ok - just adding space to the beginning of each line - although as mentioned there is a problem with that in #392... it seems wrapping is impossible...
So maybe we have this useless
Maybe it should default to
Always interesting how quickly a simple idea can get a lot more complicated ;=))
Look forward to further comments...
While this still seems a good
Accordingly, since we are about to
As always, comments, patches, PR very welcome... thanks...
Review and more RESEARCH:
Have 2nd, 3rd, 4th,... thoughts on this, and after much more testing ;=))
First my reading of the code logic was wrong in parts... But still in all tests conducted with really long php lines, already wrapped, or not, short, etc, etc, I could not make this option do anything... the default
But since the
The patch is very simple, only using
At least this causes a difference between
This is pushed to an
As stated above, this probably applies to all
As also implied, it seems these were an option to wrap these various pseudo tags, in that if
What do others think? Maybe I should just give up, and close, but this branch does do what @bfrmtx asked, I think... Thanks...
@geoffmcl, this works for me as described, but...
I poked through the source code and ran many tests prior to testing this PR, and for the life of me, there's nothing that indicated what
I’ve also done some testing with
So now I wonder this:
FWIW, the new behavior is desired and I support the PR, possibly as-is if we answer the questions above, possibly with a different option name.
But if you want to open another issue like
Or if you want to suggest that the new behaviour be implemented in a new option, like
But me, feel I have discussed, researched, this to death, and not prepared to do anything different, or any more at this time... maybe being a bit under-the-weather atm features in this... do not know...
Or just move it out to a new milestone, beyond this release, close the PR, and we do dicuss it more... it has already been open since Jul 11, 2016, and could probably wait some more...
Look forward to your choice... thanks...
I support the change, and being under the weather is making you a bit grumpy, it seems! The reason I wanted discussion was to, you know, discuss. This now puts us in a place where Tidy does something with wrap-php, but does nothing with the other options, meaning that from a user's perspective, there are inconsistencies.
"What to do" hinges on what we might want to do in the future. Do we want to deprecate the other options? That means wrap-php will stand on its own, thus avoiding any inconsistencies. Do we want the other wrap- options to do the same thing as wrap-php does, someday? Then we leave it alone. Should they be called wrap-xxx-blocks if we ever implement them? Then why not make this wrap-php-blocks?, etc.
If the vision is limited to one change, then that's not a vision and everything becomes ad hoc.
tl;dr: I'm just trying ask, where do you see the other options going?
@balthisar one last quiet, calm try before this Jul 11, 2016 issue gets moved out to the next milestone... yet again...
And I am trying to answer, that is not the issue being addressed here. If you want to address other than
Then preferable add some code changes, patch or PR to support that issue... and that might include further changes in
Can't see what I might want to do in the future! And that means open, discuss, code, and present other patches, PR's on other than
Yes, at this time it is limited to one change. Whether you call that a
But I do not think because of this one change, then everything becomes ad hoc. That seems a gross exaggeration...
And am not too concerned that some very astute user might discover there is a set of
I have spent time, discussed, and presented PR #645 just for
That does not preclude me having a vision, open another issue, discuss, and code and present something more inclusive in the future, but just not now...
And that might include some sort of renaming of it, or all of them, to
We have this code for now, just for this specific issue... merge it or not is the only reply I want...
As stated earlier, this issue has already been around for some considerable time, so skipping it for now is also no problem... thanks...