tonyhffong / Lint.jl

A lint tool for Julia code
Other
169 stars 33 forks source link

Json format return message for lintserver() #194

Closed TeroFrondelius closed 7 years ago

TeroFrondelius commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

TeroFrondelius commented 7 years ago

@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 commented 7 years ago

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

TotalVerb commented 7 years ago

CI is failing on 0.5

TeroFrondelius commented 7 years ago

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 commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

TeroFrondelius commented 7 years ago

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

Few questions:

  1. Is the "vscode" style output ok for now?
  2. Is the documentation clear enough?
  3. Can I build the documentation locally somehow?
  4. 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 commented 7 years ago

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 commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

TotalVerb commented 7 years ago

We're still using readthedocs for documentation, while most packages have moved to Documenter. So I can't help with that.

TeroFrondelius commented 7 years ago

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

coveralls commented 7 years ago

Coverage Status

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

TeroFrondelius commented 7 years ago

@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: https://github.com/TeroFrondelius/Lint.jl/compare/1ccc0e9353817de6b1e4173e72b55df195a82882...TeroFrondelius:JSON

and in linter-julia: https://github.com/TeroFrondelius/linter-julia/compare/db8daf9...JSON

TeroFrondelius commented 7 years ago

I think I found my bug: https://github.com/TeroFrondelius/Lint.jl/compare/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 commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

TeroFrondelius commented 7 years ago

@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 commented 7 years ago

Coverage Status

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

TotalVerb commented 7 years ago

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

TeroFrondelius commented 7 years ago

Yes, I will try.

TeroFrondelius commented 7 years ago

BTW I changed in REQUIRE: julia 0.5

TotalVerb commented 7 years ago

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

coveralls commented 7 years ago

Coverage Status

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