converting prettydiff to prettydiff2 #1750

prettydiff · 2017-06-30T16:34:32Z

Update prettydiff dependency from version 1.16.37 to 2.2.3.

Glavin001 · 2017-06-30T16:54:36Z

@prettydiff Awesome! Could you link any "requires Pretty Diff update" related issues for Atom-Beautify to here? I want to know which Issues I should comment on once this has been merged. Thank you!

prettydiff · 2017-06-30T17:03:15Z

Yeah, I will look for those issues as a following administrative effort over the weekend. Right now I am just trying to get a green build.

The new prettydiff2 NPM package is really just the minimum for CLI execution so it is about 1.3mb unminified instead of 23mb of the complete application.

prettydiff · 2017-06-30T17:08:41Z

The code configuration seems to be correct now as the code samples are beautified in the build. I need to review the failing samples and determine where updates need to be made.

prettydiff · 2017-06-30T22:02:37Z

@Glavin001

I am having some trouble with two unit tests.

/home/travis/build/Glavin001/atom-beautify/examples/nested-jsbeautifyrc/html-erb/expected/test.erb
/home/travis/build/Glavin001/atom-beautify/examples/simple-jsbeautifyrc/ux-markup/expected/test.ux

The first one is beautified with JS Beautify, so I don't really control that one. I did test it against Pretty Diff and it works flawless after a minor modification now in NPM.

The second one is beautified by Pretty Diff and appears to be failing because all the tag names and attribute names are converted to lowercase. This is something that happens for HTML, but I treat XML as case sensitive. I did not see the html option in the options for that test unit.

Please let me know what guidance you have.

Glavin001 · 2017-06-30T22:41:26Z

/home/travis/build/Glavin001/atom-beautify/examples/simple-jsbeautifyrc/ux-markup/expected/test.ux

The second one is beautified by Pretty Diff and appears to be failing because all the tag names and attribute names are converted to lowercase. This is something that happens for HTML, but I treat XML as case sensitive. I did not see the html option in the options for that test unit.

So the test.ux file is being detected as HTML instead of UX Markup.
Atom-Beautify's Pretty Diff does support UX Markup: https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/prettydiff.coffee#L66

We want the grammar to be UX ( ❌ ) and file extension to be .ux ( ✅ ):

atom-beautify/src/languages/ux_markup.coffee

Lines 11 to 18 in 70dbad5

    
               "UX" 
        
             ] 
        
             ### 
        
             Supported extensions 
        
             ### 
        
             extensions: [ 
        
               "ux"

I think this is the language package for Atom we would want: https://github.com/ibare/language-fuse/blob/master/grammars/ux.cson#L2

And it is missing here: https://github.com/Glavin001/atom-beautify/blob/master/spec/beautify-languages-spec.coffee#L46-L53

So I recommend:

add fuse language package to allLanguages at https://github.com/Glavin001/atom-beautify/blob/master/spec/beautify-languages-spec.coffee#L46-L53 such that the .ux files are detected as UX Markup language

Glavin001 · 2017-06-30T22:50:24Z

/home/travis/build/Glavin001/atom-beautify/examples/nested-jsbeautifyrc/html-erb/expected/test.erb

The first one is beautified with JS Beautify, so I don't really control that one. I did test it against Pretty Diff and it works flawless after a minor modification now in NPM.

Pretty Diff supports ERB: https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/prettydiff.coffee#L48
JS Beautify should not support ERB: https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/js-beautify.coffee#L9-L24

From Travis CI failing build:

--- /home/travis/build/Glavin001/atom-beautify/examples/nested-jsbeautifyrc/html-erb/expected/test.erb	beautified
+++ /home/travis/build/Glavin001/atom-beautify/examples/nested-jsbeautifyrc/html-erb/expected/test.erb	expected
@@ -1,25 +1,19 @@
-<!DOCTYPE␣html␣PUBLIC␣"-//W3C//DTD␣XHTML␣1.0␣Strict//EN"⏎
-␣␣␣␣"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">⏎
+<!DOCTYPE␣html␣PUBLIC␣"-//W3C//DTD␣XHTML␣1.0␣Strict//EN"␣"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">⏎
 ⏎
-<html␣xmlns="http://www.w3.org/1999/xhtml"␣xml:lang="en"␣lang="en">⏎
-⏎
-␣␣<head>⏎
-␣␣␣␣<meta␣http-equiv="Content-Type"␣content="text/html;␣charset=utf-8"␣/>⏎
-␣␣␣␣<title>Shopping␣List␣for⏎
-␣␣␣␣␣␣<%=␣@date.strftime('%A,␣%d␣%B␣%Y')␣%>⏎
-␣␣␣␣</title>⏎
-␣␣</head>⏎
-⏎
-␣␣<body>⏎
-␣␣␣␣<h1>Shopping␣List␣for␣<%=␣@date.strftime('%A,␣%d␣%B␣%Y')␣%></h1>⏎
-␣␣␣␣<p>You␣need␣to␣buy:</p>⏎
-␣␣␣␣<ul>⏎
-␣␣␣␣␣␣<%␣for␣@item␣in␣@items␣%>⏎
-␣␣␣␣␣␣␣␣<li>⏎
-␣␣␣␣␣␣␣␣␣␣<%=␣h(@item)␣%>⏎
-␣␣␣␣␣␣␣␣</li>⏎
-␣␣␣␣␣␣␣␣<%␣end␣%>⏎
-␣␣␣␣</ul>⏎
-␣␣</body>⏎
-⏎
\ No newline at end of file
-</html>
+␣␣␣␣<html␣xmlns="http://www.w3.org/1999/xhtml"␣xml:lang="en"␣lang="en">⏎
+␣␣␣␣␣␣␣␣<head>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣<meta␣http-equiv="Content-Type"␣content="text/html;␣charset=utf-8"/>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣<title>Shopping␣List␣for⏎
+␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣<%=␣@date.strftime('%A,␣%d␣%B␣%Y')␣%></title>⏎
+␣␣␣␣␣␣␣␣</head>⏎
+␣␣␣␣␣␣␣␣<body>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣<h1>Shopping␣List␣for⏎
+␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣<%=␣@date.strftime('%A,␣%d␣%B␣%Y')␣%></h1>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣<p>You␣need␣to␣buy:</p>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣<ul>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣<%␣for␣@item␣in␣@items␣%>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣<li><%=␣h(@item)␣%></li>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣␣<%␣end␣%>⏎
+␣␣␣␣␣␣␣␣␣␣␣␣</ul>⏎
+␣␣␣␣␣␣␣␣</body>⏎
+␣␣␣␣</html>
With options:
{
    "indent_inner_html": true,
    "indent_size": 2,
    "indent_char": " ",
    "brace_style": "collapse",
    "indent_scripts": "normal",
    "wrap_line_length": 0,
    "wrap_attributes": "auto",
    "wrap_attributes_indent_size": 2,
    "preserve_newlines": true,
    "max_preserve_newlines": 1,
    "unformatted": [
        "a",
        "abbr",
        "area",
        "audio",
        "b",
        "bdi",
        "bdo",
        "br",
        "button",
        "canvas",
        "cite",
        "code",
        "data",
        "datalist",
        "del",
        "dfn",
        "em",
        "embed",
        "i",
        "iframe",
        "img",
        "input",
        "ins",
        "kbd",
        "keygen",
        "label",
        "map",
        "mark",
        "math",
        "meter",
        "noscript",
        "object",
        "output",
        "progress",
        "q",
        "ruby",
        "s",
        "samp",
        "select",
        "small",
        "span",
        "strong",
        "sub",
        "sup",
        "svg",
        "template",
        "textarea",
        "time",
        "u",
        "var",
        "video",
        "wbr",
        "text",
        "acronym",
        "address",
        "big",
        "dt",
        "ins",
        "small",
        "strike",
        "tt",
        "pre",
        "h1",
        "h2",
        "h3",
        "h4",
        "h5",
        "h6"
    ],
    "end_with_newline": false,
    "extra_liners": [
        "head",
        "body",
        "/html"
    ],
    "disabled": false,
    "default_beautifier": "JS Beautify",
    "beautify_on_save": false
}'.

The default beautifier for ERB is Pretty Diff: https://github.com/Glavin001/atom-beautify/blob/master/src/languages/erb.coffee#L22

Looks like another problem with Atom detecting an incorrect language. We want grammar HTML (Ruby - ERB). Looks like this is the correct language package: https://github.com/atom/language-ruby/blob/master/grammars/html%20(ruby%20-%20erb).cson#L1 and https://atom.io/packages/language-ruby
However, it is already installed: https://github.com/Glavin001/atom-beautify/blob/master/spec/beautify-languages-spec.coffee#L50

So... what the heck Atom!?
Looks like someone will need to investigate why this combination of language packages says .erb files are being treated as HTML. Or maybe there is a bug in Atom-Beautify for detecting the default beautifier: https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/index.coffee#L163

Glavin001 · 2017-07-01T04:37:57Z

package.json

@@ -225,6 +225,7 @@
      "atom-beautify:beautify-language-go",
      "atom-beautify:beautify-language-golang template",
      "atom-beautify:beautify-language-fortran",
+      "atom-beautify:beautify-language-fuse",


Is Fuse the Atom-Beautify language? I thought it was UX Markup.

Note that these should be automatically generated/added, so if you find yourself adding this option, it should actually not be there 😜 .

Removing...

Glavin001 · 2017-07-01T04:40:33Z

src/languages/ux_markup.coffee

@@ -2,7 +2,7 @@ module.exports = {

  name: "UX Markup"
  namespace: "ux"
-  fallback: ['html']
+  fallback: ['xml']


This may change the behaviour for current users of UX Markup. Did this solve anything by changing it to xml?

I just figured this grammar was more closely related to XML than HTML. It did not appear to have any effect in the build.

I just figured this grammar was more closely related to XML than HTML.

This is true, however any users who are leveraging the html fallback will experience an unforeseen breaking change.

It did not appear to have any effect in the build.

Unfortunately, our builds do not have great coverage 😝 . Something I hope to address with Unibeautify.

Glavin001 · 2017-07-01T04:44:29Z

spec/beautify-languages-spec.coffee

+    "gfm", "go", "html", "html-swig", "java", "javascript",
+    "json", "less", "lua", "marko", "mustache", "objective-c",
+    "perl", "php", "python", "ruby", "sass", "sql",
+    "svg", "xml"


I forgot, it also needed to be added to Travis CI's install list: https://github.com/prettydiff/atom-beautify/blob/002b668ca23b2a0932ee3cd079c212c6f5e731d9/.travis.yml#L59
And AppVeyor: https://github.com/prettydiff/atom-beautify/blob/002b668ca23b2a0932ee3cd079c212c6f5e731d9/appveyor.yml#L136

Looks like language-fuse is missing.

I am not sure why but adding language-fuse to the build artifacts makes the build time out.

stevenzeck · 2017-10-03T15:33:45Z

@Glavin001 @prettydiff What can be done to help with this PR?

Glavin001 · 2017-10-03T15:41:48Z

Travis CI builds to pass would be a start.

Also I need time to do a code review. Sorry I haven't had a chance to yet. I'll try to bulk review and merge Pull Requests this weekend.

stevenzeck · 2017-10-04T00:48:20Z

On the erb issue, Atom is definitely selecting HTML as the grammar. Atom 1.07 seems to be the last where selectGrammar was in GrammarRegistry, so I'm kind of surprised it works at all, but I don't know the deprecations and Atom API's well yet. It looks to be replaced by grammarForScopeName. No idea how to actually get the scope for a package via the API, it is listed in the settings for each language package.

EDIT: Confirmed, I manually stated to use HTML (Ruby - ERB) grammar for the test.erb file, and Pretty Diff beautified it. @prettydiff it still failed though, the original versus expected indentations aren't the same, here is what beautifying the original resulted in:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
        <title>Shopping List for
            <%= @date.strftime('%A, %d %B %Y') %></title>
    </head>
    <body>
        <h1>Shopping List for
            <%= @date.strftime('%A, %d %B %Y') %></h1>
        <p>You need to buy:</p>
        <ul>
            <% for @item in @items %>
                <li><%= h(@item) %></li>
            <% end %>
        </ul>
    </body>
</html>

stevenzeck · 2017-10-04T06:09:16Z

For the UX issue, I think the reason it times out is because it's not being activated properly, see https://discuss.atom.io/t/how-do-i-activate-a-package-in-specs/18766. Is it being tested right now? If so, how?

Going to bed now.

EDIT: I tried the fuse package, and although I get fuse could not be spawned when installing the package, the tests do run. But the all the tag and attribute names are still being converted to lower case, despite it recognizing the grammar/language/namespace as UX/UX Markup/ux and Pretty Diff being the beautifier.

So if language-fuse is the one to use, it needs to be activated differently than the others based on the above link. @Glavin001 Do you know how to do that?

Update 2: I opened an issue on language-fuse, it hasn't been updated in 2 years so don't know if there will be a response. I don't see a purpose in having an activationCommand in package.json for that package.

prettydiff · 2017-10-04T17:43:28Z

@szeck87 The beautified ERB output looks correct to me. I am looking at the test file and it appears the test file is wrong. There is an extra step of indentation as though the doctype is a start tag: https://github.com/Glavin001/atom-beautify/blob/master/examples/nested-jsbeautifyrc/html-erb/expected/test.erb

stevenzeck · 2017-10-04T17:59:18Z

@prettydiff Sorry, I'm actually going off of the prettydiff2 branch from this PR: https://github.com/prettydiff/atom-beautify/blob/prettydiff2/examples/nested-jsbeautifyrc/html-erb/expected/test.erb. Beautifying the original file from that branch doesn't match the indentations there in the expected.

prettydiff · 2017-10-04T21:11:58Z

@szeck87 I updated that test sample in my fork.

stevenzeck · 2017-10-04T22:34:21Z

@prettydiff Looks good, the erb test passed. I think the only issue left, apart from the fuse packages and getting the proper grammar correctly which are both Atom issues, is the UX file test going to lowercase. I did confirm that if you install language-fuse or fuse as an Atom package and beautify those files, it does treat them as UX and UX Markup for the grammar and language respectfully and Pretty Diff as the beautifier, so something is still off there. I noticed that it sets html as the lang in prettydiff.coffee, is that supposed to be? https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/prettydiff.coffee#L96-L97

stevenzeck · 2017-10-04T23:53:27Z

The erb files just need to be renamed from test.erb to test.html.erb, then they will be interpreted as HTML (Ruby - ERB) instead of HTML or HTML (Rails).

prettydiff · 2017-10-05T02:38:54Z

@szeck87 Files renamed.

Glavin001 · 2017-10-06T02:19:45Z

package.json

@@ -175,7 +175,7 @@
    "node-dir": "0.1.17",
    "node-uuid": "1.4.8",
    "open": "0.0.5",
-    "prettydiff": "1.16.37",
+    "prettydiff2": ">2.2.5",


Does prettydiff2 follow semver? If so, this should be ^2.2.5 so no major version is updated.

Glavin001 · 2017-10-06T02:24:55Z

src/languages/ux_markup.coffee

@@ -2,7 +2,7 @@ module.exports = {

  name: "UX Markup"
  namespace: "ux"
-  fallback: ['html']
+  fallback: ['xml']


I just figured this grammar was more closely related to XML than HTML.

This is true, however any users who are leveraging the html fallback will experience an unforeseen breaking change.

It did not appear to have any effect in the build.

Unfortunately, our builds do not have great coverage 😝 . Something I hope to address with Unibeautify.

Glavin001 · 2017-10-06T02:32:41Z

Restarting build. Let's see how it goes.

Glavin001 · 2017-10-07T17:21:12Z

I created a new PR where I can push changes: #1878

converting prettydiff to prettydiff2

d57c90c

prettydiff added the add-beautifier label Jun 30, 2017

Austin Cheney added 2 commits June 30, 2017 11:41

fixing a 1 character typo

7178248

fixing a 1 character typo

dc36110

Glavin001 self-requested a review June 30, 2017 16:53

Glavin001 self-assigned this Jun 30, 2017

Glavin001 added the high priority label Jun 30, 2017

Glavin001 added this to the v0.31.0 milestone Jun 30, 2017

adding a necessary missing lib to prettydiff

9e8f64f

Austin Cheney added 2 commits June 30, 2017 16:01

updating code samples to match updated pretty diff

9f44a63

still working the prettydiff2

c9f2f1f

Austin Cheney added 3 commits June 30, 2017 19:41

adjustments from Glavin's guidance

46e0274

update

10c5e26

adding language-fuse to package.json

002b668

Glavin001 requested changes Jul 1, 2017

View reviewed changes

Austin Cheney added 3 commits July 1, 2017 07:00

adding flux as a language dependency for CI builds

9de6c85

should have been language-fuse

9d9d833

removing added line from package.json per guidance

06dd035

This was referenced Jul 30, 2017

Add YAML Front Matter in CSS/SCSS support (Jekyll) #861

Closed

Beautification of comment in combination with language-babel package breaks code #1789

Closed

This was referenced Aug 8, 2017

Text change to "Error: source does not appear to be markup." prettydiff/prettydiff#472

Closed

.tsx support #601

Closed

prettydiff mentioned this pull request Aug 20, 2017

<template> elements not handled properly #1583

Closed

5 tasks

prettydiff mentioned this pull request Sep 6, 2017

Preserve newlines in CSS prettydiff/prettydiff#478

Closed

prettydiff mentioned this pull request Oct 2, 2017

property deleted by atom-beautify #1835

Closed

5 tasks

updating a test sample

25c422c

renaming test files so that erb is beautified as html

f908305

Glavin001 added the in-progress label Oct 6, 2017

Glavin001 requested changes Oct 6, 2017

View reviewed changes

Glavin001 removed high priority in-progress labels Oct 8, 2017

Glavin001 closed this Oct 8, 2017

This was referenced Apr 7, 2022

[Snyk] Fix for 1 vulnerabilities #2617

Open

[Snyk] Fix for 1 vulnerabilities ra100/atom-beautify#1

Merged

[Snyk] Fix for 1 vulnerabilities edwinksl/atom-beautify#12

Open

[Snyk] Fix for 1 vulnerabilities cooljl31/atom-beautify#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

converting prettydiff to prettydiff2 #1750

converting prettydiff to prettydiff2 #1750

prettydiff commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

prettydiff commented Jun 30, 2017

prettydiff commented Jun 30, 2017

prettydiff commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

Glavin001 Jul 1, 2017

prettydiff Jul 1, 2017

Glavin001 Jul 1, 2017

prettydiff Jul 1, 2017

Glavin001 Oct 6, 2017

Glavin001 Jul 1, 2017

prettydiff Jul 1, 2017

stevenzeck commented Oct 3, 2017

Glavin001 commented Oct 3, 2017

stevenzeck commented Oct 4, 2017 •

edited

Loading

stevenzeck commented Oct 4, 2017 •

edited

Loading

prettydiff commented Oct 4, 2017

stevenzeck commented Oct 4, 2017

prettydiff commented Oct 4, 2017

stevenzeck commented Oct 4, 2017 •

edited

Loading

stevenzeck commented Oct 4, 2017

prettydiff commented Oct 5, 2017

Glavin001 Oct 6, 2017

Glavin001 Oct 6, 2017

Glavin001 commented Oct 6, 2017

Glavin001 commented Oct 7, 2017

converting prettydiff to prettydiff2 #1750

converting prettydiff to prettydiff2 #1750

Conversation

prettydiff commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

prettydiff commented Jun 30, 2017

prettydiff commented Jun 30, 2017

prettydiff commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

Glavin001 commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzeck commented Oct 3, 2017

Glavin001 commented Oct 3, 2017

stevenzeck commented Oct 4, 2017 • edited Loading

stevenzeck commented Oct 4, 2017 • edited Loading

prettydiff commented Oct 4, 2017

stevenzeck commented Oct 4, 2017

prettydiff commented Oct 4, 2017

stevenzeck commented Oct 4, 2017 • edited Loading

stevenzeck commented Oct 4, 2017

prettydiff commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glavin001 commented Oct 6, 2017

Glavin001 commented Oct 7, 2017

stevenzeck commented Oct 4, 2017 •

edited

Loading

stevenzeck commented Oct 4, 2017 •

edited

Loading

stevenzeck commented Oct 4, 2017 •

edited

Loading