Json format return message for lintserver() #194

TeroFrondelius · 2017-02-20T15:44:52Z

This pull request is ready.

TODO:

See issue #131
Maybe it's ok to list the old issue here to get it closed: #70

@TotalVerb and @davidanthoff your comments / improvement ideas would be appreciated.

Ps. I still haven't learned the git. Now it shows all the commits also for the previous pull request.

davidanthoff · 2017-02-20T18:07:38Z

@TeroFrondelius Can you ping me again when this is ready to be reviewed? I don't have much time these days, so would prefer to look at a final version.

TeroFrondelius · 2017-02-20T19:50:18Z

@davidanthoff sure I will ping you again. Now there is only a fundamental question: should I keep as it is, or should I change that all messages are sent as one json. Now: {file: 'none', code: 'E321' ...}, other option: [{file: 'none', code: 'E321' ...}, {file: 'none', code: 'E111' ...}], this second option good side is that it would be already in the format linter is expecting it. I'm just thinking do I gain anything if I have to filter the messages in linter-julia side anyways (like ignoring some error codes).

davidanthoff · 2017-02-20T19:55:44Z

I think we would want to go with one JSON that has an array of messages, right?

TeroFrondelius · 2017-02-20T20:06:23Z

@davidanthoff yes this is my second option. Thus you think it's better? I think it should be relatively easy to implement, I just need second opinion, because I haven't decided which one is better.

davidanthoff · 2017-02-20T20:20:09Z

Yes, I think that is better, otherwise we need another "protocol" that defines how different JSON messages are combined.

TotalVerb

Good contribution! Just a few comments.

TotalVerb · 2017-02-21T04:21:26Z

src/Lint.jl

@@ -350,7 +356,54 @@ function lintdir{T<:AbstractString}(dir::T, ctx::LintContext=LintContext())
    ctx.messages
 end

-function readandwritethestream(conn)
+function convertmsgtojson(msgs, style)
+    if style == "LintMessage"


This style is in CamelCase and the others in snake-case; should be made consistent.

Fixed in the next commit.

TotalVerb · 2017-02-21T04:22:37Z

src/Lint.jl

+        txt = msg.message
+        file = msg.file
+        linenumber = msg.line
+        errorrange = Array[[linenumber, 0], [linenumber, 80]]


We don't support 0.4 any more so this should be [[linenumber, 0], [linenumber, 80]], i.e. leave out the Array.

See issue #195

OK, we can leave this until that is fixed.

TotalVerb · 2017-02-21T04:24:15Z

src/Lint.jl

 end

-function lintserver(port)
+function lintserver(port,style)


Use default argument; i.e. style="original behavior".

Fixed in the next commit.

TotalVerb · 2017-02-21T04:24:32Z

src/Lint.jl

@@ -391,6 +449,10 @@ function lintserver(port)
    end
 end

+function lintserver(port)


as above, use default argument instead of this

Fixed in the next commit.

TotalVerb · 2017-02-21T22:26:54Z

CI is failing on 0.5

TotalVerb · 2017-02-21T22:27:30Z

test/server.jl

+    socket = connect(pipe)
+    write(socket, json_input * "\n")
+    json_output = readline(socket)
+    results_array = JSON.parse(strip(json_output))


don't need to strip json, here and elsewhere.

Thanks, I forgot this one.

TotalVerb · 2017-02-21T22:29:16Z

src/Lint.jl

        write(conn, "\n")
+    else
+        json_data = readline(conn)
+        dict_data = JSON.parse(json_data)


I believe you can parse the connection directly; i.e. JSON.parse(conn) instead of JSON.parse(readline(conn)). I recommend that over readline so the client can send newlines in their JSON if they prefer.

Thanks, I really appreciate your effort to review and improve my code.

TeroFrondelius · 2017-02-22T06:53:54Z

I guess CI is failing, because I haven't updated the REQUIRE file to demand JSON dependency. I will add it to next commit.

coveralls · 2017-02-22T10:18:35Z

Coverage increased (+0.3%) to 86.789% when pulling 39c50f5 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

…message text

coveralls · 2017-02-22T12:54:32Z

Coverage increased (+0.2%) to 86.68% when pulling a81d069 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-02-22T14:31:09Z

Coverage increased (+0.3%) to 86.821% when pulling c5ce694 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-02-25T14:31:54Z

Coverage increased (+0.3%) to 86.821% when pulling ddefddb on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

TeroFrondelius · 2017-02-25T14:43:53Z

@davidanthoff and @TotalVerb this is ready for the final review.

Few questions:

Is the "vscode" style output ok for now?
Is the documentation clear enough?
Can I build the documentation locally somehow?
Is the git status ok? I tried following, but it didn't help:

git remote add upstream https://github.com/tonyhffong/Lint.jl.git
git checkout master
git fetch upstream
git merge upstream/master
git push

TeroFrondelius · 2017-02-25T16:43:41Z

FYI: here is the linter-julia code version that uses this version: https://github.com/TeroFrondelius/linter-julia/blob/JSON/lib/linter-julia.coffee

coveralls · 2017-02-25T20:05:19Z

Coverage increased (+0.3%) to 86.821% when pulling 343f842 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-02-25T20:05:19Z

Coverage increased (+0.3%) to 86.821% when pulling 343f842 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

TeroFrondelius · 2017-02-27T19:20:23Z

I now have a problem with linter-julia json parsing. I need to study what changed.

coveralls · 2017-02-27T20:49:22Z

Coverage increased (+0.3%) to 86.821% when pulling 0222c9d on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

TotalVerb

The code LGTM now. I just have a few remaining comments about the documentation, which is overall great.

TotalVerb · 2017-03-01T19:09:27Z

docs/features.md

+The new protocol for the server is JSON in both input and output:
+```json
+"{
+    \"file\":\"path_to_the_file\",


This is a Julia string that happens to contain JSON, not JSON. We should not have the \" and the surrounding "" in the README.

This is fixed.

TotalVerb · 2017-03-01T19:10:12Z

docs/features.md

+    \"show_code\":true
+}"
+```
+Only the two first `'file'` and `'code_str'` are mandatory fields. For the output


should be "file" and "code_str"

This is fixed.

TotalVerb · 2017-03-01T19:10:46Z

docs/features.md

+```
+Only the two first `'file'` and `'code_str'` are mandatory fields. For the output
+there are four different protocols from which the `"lint-message"` is preferred,
+because it is the direct match of `LintMessage` and this way will be always up


I wouldn't say it is preferred, because the LintMessage format, while up to date, is subject to change.

TotalVerb · 2017-03-01T19:12:30Z

docs/features.md

+julia> conn = connect(pipe_lm)
+Base.PipeEndpoint(open, 0 bytes waiting)
+
+julia> write(conn, json_input1)


I think using JSON.print(conn, Dict(...)) in this example would be better.

For some reason this didn't work:

julia> conn = connect(pipe_lm) Base.PipeEndpoint(open, 0 bytes waiting) julia> JSON.print(conn, json_input1) connection ended with error MethodError(getindex,("{\"file\":\"none\",\"code_str\":\"something\"}","file")) julia> conn = connect(pipe_lm) Base.PipeEndpoint(open, 0 bytes waiting) julia> write(conn, json_input1) 38

Sorry for the noise. I found my mistake. I will push another commit.

TeroFrondelius · 2017-03-02T15:19:15Z

@TotalVerb thanks, once again, your valuable comments. I will make the changes. If you wouldn't mind taking another look (or even testing) where is my bug. I had this already working with linter-julia, but then I introduced a bug somewhere, which I haven't been able to find. It can be trivial because there are not so many changes between the working commits and non working.

This is the comparison to working version and current, changes in Lint.jl:
TeroFrondelius/Lint.jl@1ccc0e9...TeroFrondelius:JSON

and in linter-julia: AtomLinter/linter-julia@db8daf9...JSON

TeroFrondelius · 2017-03-04T15:42:51Z

I think I found my bug: TeroFrondelius/Lint.jl@a81d069...TeroFrondelius:ddefddb

if haskey(dict_data,"show_code")
    if !dict_data["show_code"]
        msgtext = "$evar: $txt"
    end
else
    msgtext = "$code $evar: $txt"
end

The bug is the case when "show_code" = true

coveralls · 2017-03-04T16:35:10Z

Coverage increased (+0.3%) to 86.778% when pulling 91dd2f0 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-03-04T17:06:50Z

Coverage increased (+0.3%) to 86.778% when pulling 175ab5c on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-03-05T10:42:58Z

Coverage increased (+0.3%) to 86.828% when pulling f814b29 on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

coveralls · 2017-03-05T10:56:57Z

Coverage increased (+0.3%) to 86.828% when pulling b4593be on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

TeroFrondelius · 2017-03-05T11:02:56Z

@tonyhffong or @Michael-Klassen, I think this is ready for merge. @TotalVerb thanks for your help, please comment if there is still something to do.

coveralls · 2017-03-05T16:49:30Z

Coverage increased (+0.3%) to 86.828% when pulling 78dfe6c on TeroFrondelius:JSON into f1db4f0 on tonyhffong:master.

TotalVerb

Thanks. @tonyhffong @Michael-Klassen This LGTM.

TotalVerb · 2017-03-06T18:39:59Z

@TeroFrondelius The merging of #179 has resulted in some merge conflicts; could you resolve those?

TeroFrondelius · 2017-03-06T19:02:49Z

Yes, I will try.

TeroFrondelius · 2017-03-06T19:36:39Z

BTW I changed in REQUIRE: julia 0.5

TotalVerb · 2017-03-06T22:39:39Z

For some reason, Travis did not run. Could you try closing and reopening this PR to restart the CI?

coveralls · 2017-03-07T06:50:51Z

Coverage increased (+0.4%) to 88.912% when pulling 38ccf65 on TeroFrondelius:JSON into 564469e on tonyhffong:master.

TeroFrondelius added 3 commits February 19, 2017 22:00

convertmsgtojson() added

4242143

Adding also LintMessage style to convertmsgtojson()

6ca5e53

Adding tests

0c68068

Changing lintserver() to return single json

ab52c79

TotalVerb requested changes Feb 21, 2017

View reviewed changes

TeroFrondelius added 3 commits February 21, 2017 12:36

Fixes according to @TotalVerb review

8351265

Updating lintserver() receive side to accept JSON

f4440a6

Fine tuning after testing with linter-julia

1ccc0e9

TotalVerb requested changes Feb 21, 2017

View reviewed changes

Updates based on @TotalVerb comments

39c50f5

Adding the feature to turn off the codes and some fine tuning of the …

a81d069

…message text

Adding the missing test to previous commit

c5ce694

Writing the new server protocol documentation to features.md

ddefddb

More text at features.md

343f842

TeroFrondelius mentioned this pull request Feb 26, 2017

Tag a release #191

Closed

changes according to review

0222c9d

TeroFrondelius mentioned this pull request Feb 27, 2017

Follow the lintserver() message development towards JSON AtomLinter/linter-julia#4

Closed

TotalVerb reviewed Mar 1, 2017

View reviewed changes

TeroFrondelius added 2 commits March 4, 2017 17:54

Fixing the standard-linter-v1 message bug when show_code = true

8e268fb

Documentation update based on @TotalVerb review

91dd2f0

Documentation update based on @TotalVerb review, fixing the example

175ab5c

Adding the test for the bug: "show_code"=true

f814b29

tets/server.jl updated to use JSON.print style

b4593be

lintdir() doc string rewording to remove asterisk sign

78dfe6c

TotalVerb approved these changes Mar 6, 2017

View reviewed changes

resolving merge conflict

38ccf65

TeroFrondelius closed this Mar 7, 2017

TeroFrondelius reopened this Mar 7, 2017

Michael-Klassen merged commit 1dfcfd4 into tonyhffong:master Mar 7, 2017

TotalVerb mentioned this pull request Mar 19, 2017

Change format of lintserver return messages #131

Closed

Json format return message for lintserver() #194

Json format return message for lintserver() #194

Conversation

TeroFrondelius commented Feb 20, 2017 • edited Loading

davidanthoff commented Feb 20, 2017

TeroFrondelius commented Feb 20, 2017

davidanthoff commented Feb 20, 2017

TeroFrondelius commented Feb 20, 2017

davidanthoff commented Feb 20, 2017

TotalVerb left a 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

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

TotalVerb commented Feb 21, 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

TeroFrondelius commented Feb 22, 2017

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 25, 2017 • edited Loading

TeroFrondelius commented Feb 25, 2017 • edited Loading

TeroFrondelius commented Feb 25, 2017

coveralls commented Feb 25, 2017

coveralls commented Feb 25, 2017

TeroFrondelius commented Feb 27, 2017

coveralls commented Feb 27, 2017 • edited Loading

TotalVerb left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeroFrondelius commented Mar 2, 2017

TeroFrondelius commented Mar 4, 2017

coveralls commented Mar 4, 2017 • edited Loading

coveralls commented Mar 4, 2017 • edited Loading

coveralls commented Mar 5, 2017 • edited Loading

coveralls commented Mar 5, 2017 • edited Loading

TeroFrondelius commented Mar 5, 2017

coveralls commented Mar 5, 2017 • edited Loading

TotalVerb left a comment

Choose a reason for hiding this comment

TotalVerb commented Mar 6, 2017

TeroFrondelius commented Mar 6, 2017

TeroFrondelius commented Mar 6, 2017

TotalVerb commented Mar 6, 2017

coveralls commented Mar 7, 2017 • edited Loading

TeroFrondelius commented Feb 20, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 25, 2017 •

edited

Loading

TeroFrondelius commented Feb 25, 2017 •

edited

Loading

coveralls commented Feb 27, 2017 •

edited

Loading

coveralls commented Mar 4, 2017 •

edited

Loading

coveralls commented Mar 4, 2017 •

edited

Loading

coveralls commented Mar 5, 2017 •

edited

Loading

coveralls commented Mar 5, 2017 •

edited

Loading

coveralls commented Mar 5, 2017 •

edited

Loading

coveralls commented Mar 7, 2017 •

edited

Loading