mohkale / flymake-collection

Collection of checkers for flymake
MIT License
68 stars 13 forks source link

flymake-collection-parse-json error with reading JSON from Credo #24

Closed alecStewart1 closed 1 year ago

alecStewart1 commented 1 year ago

Hello!

I'm trying to create a checker for Credo for Elixir.

Still figuring a few things about, but when I try to use flymake-collection-parse-json with the JSON from here I get: *** Eval error *** could not parse JSON stream: "unexpected newline", "<callback>", 7, 104, 252. I did this in ielm.

 (flymake-collection-parse-json "{
  \"explanations\": [
    {
      \"category\": \"refactor\",
      \"check\": \"Elixir.Foo.Bar.Refactor.NegatedConditionsWithElse\",
      \"column\": null,
      \"explanation_for_issue\": \"An `if` block with a negated condition should not contain an else block.\n\nSo while this is fine:\n\n    if not allowed? do\n      raise \"Not allowed!\"\n    end\n\nThe code in this example ...\n\n    if not allowed? do\n      raise \"Not allowed!\"\n    else\n      proceed_as_planned()\n    end\n\n... should be refactored to look like this:\n\n    if allowed? do\n      proceed_as_planned()\n    else\n      raise \"Not allowed!\"\n    end\n\nThe same goes for negation through `!` instead of `not`.\n\nThe reason for this is not a technical but a human one. It is easier to wrap\nyour head around a positive condition and then thinking \"and else we do ...\".\n\nIn the above example raising the error in case something is not allowed\nmight seem so important to put it first. But when you revisit this code a\nwhile later or have to introduce a colleague to it, you might be surprised\nhow much clearer things get when the \"happy path\" comes first.\n\",
      \"filename\": \"lib/foo/bar.ex\",
      \"line_no\": 306,
      \"message\": \"Avoid negated conditions in if-else blocks.\",
      \"priority\": 12,
      \"related_code\": [
        [304, \"    explanation = Module.get_attribute(env.module, explanation)\"],
        [305, \"\"],
        [306, \"    if not is_nil(explanation) do\"],
        [307, \"      # deprecated - remove once we ditch @explanation\"],
        [308, \"      quote do\"]
      ],
      \"scope\": \"Foo.Bar.deprecated_def_explanations\",
      \"trigger\": \"!\"
    }
  ]
}")

I don't know if this would be an issue when actually doing the parsing for the checker, but I would like to parse this output to understand what I need to do for the :generator for flymake-collection-define-enumerate.

mohkale commented 1 year ago

You can replace with insert to see what the actual JSON string passed to parse-json will be:

(insert "{
  \"explanations\": [
    {
      \"category\": \"refactor\",
      \"check\": \"Elixir.Foo.Bar.Refactor.NegatedConditionsWithElse\",
      \"column\": null,
      \"explanation_for_issue\": \"An `if` block with a negated condition should not contain an else block.\n\nSo while this is fine:\n\n    if not allowed? do\n      raise \"Not allowed!\"\n    end\n\nThe code in this example ...\n\n    if not allowed? do\n      raise \"Not allowed!\"\n    else\n      proceed_as_planned()\n    end\n\n... should be refactored to look like this:\n\n    if allowed? do\n      proceed_as_planned()\n    else\n      raise \"Not allowed!\"\n    end\n\nThe same goes for negation through `!` instead of `not`.\n\nThe reason for this is not a technical but a human one. It is easier to wrap\nyour head around a positive condition and then thinking \"and else we do ...\".\n\nIn the above example raising the error in case something is not allowed\nmight seem so important to put it first. But when you revisit this code a\nwhile later or have to introduce a colleague to it, you might be surprised\nhow much clearer things get when the \"happy path\" comes first.\n\",
      \"filename\": \"lib/foo/bar.ex\",
      \"line_no\": 306,
      \"message\": \"Avoid negated conditions in if-else blocks.\",
      \"priority\": 12,
      \"related_code\": [
        [304, \"    explanation = Module.get_attribute(env.module, explanation)\"],
        [305, \"\"],
        [306, \"    if not is_nil(explanation) do\"],
        [307, \"      # deprecated - remove once we ditch @explanation\"],
        [308, \"      quote do\"]
      ],
      \"scope\": \"Foo.Bar.deprecated_def_explanations\",
      \"trigger\": \"!\"
    }
  ]
}")

In this case it's:

{
  "explanations": [
    {
      "category": "refactor",
      "check": "Elixir.Foo.Bar.Refactor.NegatedConditionsWithElse",
      "column": null,
      "explanation_for_issue": "An `if` block with a negated condition should not contain an else block.

So while this is fine:

    if not allowed? do
      raise "Not allowed!"
    end

The code in this example ...

    if not allowed? do
      raise "Not allowed!"
    else
      proceed_as_planned()
    end

... should be refactored to look like this:

    if allowed? do
      proceed_as_planned()
    else
      raise "Not allowed!"
    end

The same goes for negation through `!` instead of `not`.

The reason for this is not a technical but a human one. It is easier to wrap
your head around a positive condition and then thinking "and else we do ...".

In the above example raising the error in case something is not allowed
might seem so important to put it first. But when you revisit this code a
while later or have to introduce a colleague to it, you might be surprised
how much clearer things get when the "happy path" comes first.
",
      "filename": "lib/foo/bar.ex",
      "line_no": 306,
      "message": "Avoid negated conditions in if-else blocks.",
      "priority": 12,
      "related_code": [
        [304, "    explanation = Module.get_attribute(env.module, explanation)"],
        [305, ""],
        [306, "    if not is_nil(explanation) do"],
        [307, "      # deprecated - remove once we ditch @explanation"],
        [308, "      quote do"]
      ],
      "scope": "Foo.Bar.deprecated_def_explanations",
      "trigger": "!"
    }
  ]
}

Which is not valid JSON so parsing fails.

alecStewart1 commented 1 year ago

Hmmm well is there a way to make sure that the newlines are kept in order for the JSON to be valid?

alecStewart1 commented 1 year ago

Well here's what I gathered from looking at other checkers and what's in flymake-credo

(flymake-collection-define-enumerate flymake-collection-credo
  "Credo is a static code analysis tool for the Elixir language with a focus on teaching
and code consistency.

See https://github.com/rrrene/credo for the project README and
https://hexdocs.pm/credo/overview.html for the HexDocs."
  :title "credo"
  :pre-let ((credo
             (if (project-root (project-current))
                 (list (executable-find "mix") "credo")
               (list (executable-find "credo")))))
  :pre-check (unless (car credo)
               (error "Cannot find executable for credo"))
  :write-type 'pipe
  :command
  `(,@credo
    "list"
    ,@(when-let ((file (buffer-file-name flymake-collection-source)))
        file)
    "--format json"
    "--read-from-stdin")
  :generator
  (alist-get
   'issues
   (caar
    (flymake-collection-parse-json
     (buffer-substring-no-properties
      (point-min) (point-max)))))
  :enumerate-parser
  (let-alist it
    (let* ((base-loc (flymake-diag-region flymake-collection-source .line_no .column))
           (loc (cons (car base-loc)
                      (cdr
                       (if (and .endLine .endColumn)
                           (flymake-diag-region flymake-collection-source
                                                .endLine (1- .endColumn))
                          base-loc)))))
      (list
       flymake-collection-source
       (car loc)
       (cdr loc)
       :error
       (concat .message " [Priority: " .priority "]")))))
mohkale commented 1 year ago

Hmmm well is there a way to make sure that the newlines are kept in order for the JSON to be valid?

You can escape them correctly. \\n instead of \n.

mohkale commented 1 year ago

Well here's what I gathered from looking at other checkers and what's in flymake-credo

(flymake-collection-define-enumerate flymake-collection-credo
  "Credo is a static code analysis tool for the Elixir language with a focus on teaching
and code consistency.

See https://github.com/rrrene/credo for the project README and
https://hexdocs.pm/credo/overview.html for the HexDocs."
  :title "credo"
  :pre-let ((credo
             (if (project-root (project-current))
                 (list (executable-find "mix") "credo")
               (list (executable-find "credo")))))
  :pre-check (unless (car credo)
               (error "Cannot find executable for credo"))
  :write-type 'pipe
  :command
  `(,@credo
    "list"
    ,@(when-let ((file (buffer-file-name flymake-collection-source)))
        file)
    "--format json"
    "--read-from-stdin")
  :generator
  (alist-get
   'issues
   (caar
    (flymake-collection-parse-json
     (buffer-substring-no-properties
      (point-min) (point-max)))))
  :enumerate-parser
  (let-alist it
    (let* ((base-loc (flymake-diag-region flymake-collection-source .line_no .column))
           (loc (cons (car base-loc)
                      (cdr
                       (if (and .endLine .endColumn)
                           (flymake-diag-region flymake-collection-source
                                                .endLine (1- .endColumn))
                          base-loc)))))
      (list
       flymake-collection-source
       (car loc)
       (cdr loc)
       :error
       (concat .message " [Priority: " .priority "]")))))

Great. Please open PR. Tests are appreciated.