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

fix #906. New unformatted_tag_content option. #1081

Closed
wants to merge 3 commits into from
Closed

fix #906. New unformatted_tag_content option. #1081

wants to merge 3 commits into from

Conversation

mykytak
Copy link

@mykytak mykytak commented Dec 25, 2016

Fix for #906

New option for unformatted content: unformatted_tag_content. Usage same as for unformatted, console short version: -utc.

Parser now has new tag_type: UNFORMATTED. It parses content inside tag but process tag itself as separate tag_type instead of getting them as raw text.

@bitwiseman
Copy link
Member

@mykytak - #1080 was submitted first, so I'm going to evaluate their PR first. You have better tests but also a more complex implementation.

@mykytak
Copy link
Author

mykytak commented Dec 26, 2016

@bitwiseman His solution is not working with js/lib/cli.js calls. I got same 4 days ago and did'n push because of this. Tests run ok, beautify-html run ok but cli fails (didn't set \n before closing body tag). That's why my solution is more complex.

@bitwiseman
Copy link
Member

@mykytak - please provide an example? How can I see the failure?

@mykytak
Copy link
Author

mykytak commented Dec 26, 2016

@bitwiseman You can use code from #1080, I got same code and same result.
test.html:

<body><h1>Heading</h1><script>var formatMe = function() { return false; };</script><style>.format-disabled { display: none; } </style></body>

run $ node js/lib/cli.js -T script -T style test.html -o res.html
after this res.html will be

<body>
    <h1>Heading</h1>
    <script>var formatMe = function() { return false; };</script>
    <style>.format-disabled { display: none; } </style></body>

@bitwiseman
Copy link
Member

When I run the test you outlined on ( b3798f1) it comes out:

<body>
    <h1>Heading</h1>
    <script>var formatMe = function() { return false; };</script>
    <style>.format-disabled { display: none; } </style>
</body>

What platform are you running on?

@bitwiseman
Copy link
Member

(Note: I did find an unrelated bug while testing this, so thank you there. 😄 )

@mykytak
Copy link
Author

mykytak commented Dec 27, 2016

@bitwiseman Ubuntu14, 64bit

@mykytak
Copy link
Author

mykytak commented Dec 27, 2016

@bitwiseman I'll send you virtualbox files.

@bitwiseman
Copy link
Member

@mykytak - Could you export JSBEAUTIFY_DEBUG=yes and then send me the debug output? That should take less time.

@mykytak
Copy link
Author

mykytak commented Dec 27, 2016

@bitwiseman

executable type: cli.js
parsed type: undefined
type defaulted: cli.js
files.length 1
{ content_unformatted: [ 'script', 'style' ],
  outfile: '/vagrant/res.html',
  argv: 
   { remain: [ 'test.html' ],
     cooked: 
      [ '--content_unformatted',
        'script',
        '--content_unformatted',
        'style',
        'test.html',
        '--outfile',
        'res.html' ],
     original: [ '-T', 'script', '-T', 'style', 'test.html', '-o', 'res.html' ] },
  indent_size: 4,
  indent_char: ' ',
  indent_level: 0,
  indent_with_tabs: false,
  preserve_newlines: true,
  max_preserve_newlines: 10,
  jslint_happy: false,
  space_after_anon_function: false,
  brace_style: 'collapse',
  keep_array_indentation: false,
  keep_function_indentation: false,
  space_before_conditional: true,
  break_chained_methods: false,
  eval_code: false,
  unescape_strings: false,
  wrap_line_length: 0,
  type: 'cli.js',
  files: [ '/vagrant/test.html' ] }
beautified res.html

@bitwiseman
Copy link
Member

Hmm, I was wrong, no help there.

@mykytak
Copy link
Author

mykytak commented Dec 27, 2016

@bitwiseman check your inbox, I create ssh tunnel into my machine and sent you instructions.

@bitwiseman
Copy link
Member

bitwiseman commented Dec 27, 2016

@mykytak - looked. I can see the repro, but I'm not spotting the source of the issue. I'm not sure why it would only show up in the CLI.

Tried node 0.10.48 on OSX - no repro.

It is late here, I'll take a look at this in the next couple days.

@arai-a
Copy link
Contributor

arai-a commented Dec 27, 2016

what happens if you use -U instead of -T ?
newline after tag should be handled samely between unformatted and content_unformatted.
do you get newline after </style> ?

@arai-a
Copy link
Contributor

arai-a commented Dec 27, 2016

sorry, I meant, run without -T, not with -U
(I was thinking in reverse way

adding -T shouldn't change the handling of newline after tag

@bitwiseman
Copy link
Member

@mykytak - I can't repro this except on the vm you provided. It certainly shows a shortcoming of the debug output that I can't figure out why the outputs differ, but that is a separate issue. I'm going to accept #1080.

@bitwiseman bitwiseman closed this Dec 28, 2016
@bitwiseman
Copy link
Member

I'm sorry you feel that your PR was not given the appropriate consideration. I hope you will continue to contribute beyond this one PR.

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