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

Improved lint report readability #598

Merged
merged 10 commits into from
Jun 27, 2016

Conversation

pcantrell
Copy link
Collaborator

@pcantrell pcantrell commented Jun 23, 2016

This PR contains three improvements to make undocumented.json more useful & more human-readable:

  • The file is now pretty printed.
  • Warnings are sorted by (file, line) instead of the current somewhat haphazard ordering.
  • There is a new symbol_qualified key that gives, for example, "Error.Cause.UnencodableText.text" instead of the no-so-useful "text". (In a green field, I’d prefer to just replacing the existing symbol with that fully qualified value, but I left the old key/value intact in case anyone’s using it.)

Here are the results.

I know the intent was to run undocumented.json through some reporting tool, but it’s nice to also be able to read the file itself.

In the course of implementing this, I noticed that jazzy was generating the lint report using the raw Sourcekitten dict, which resulted in some redundant logic. I switched over to using the parsed SourceDeclaration object, and that cleaned up a few dozen lines of code. I put that change, the one that caused the most code disruption, in its own commit, and verified that it didn’t alter the integration specs. (I did, however, get different spec output locally than was checked in even before making changes. Dependencies changed? Different Ruby? Different Sourcekitten? We’ll see what happens on CI….)

@pcantrell
Copy link
Collaborator Author

Oh, also worth mentioning: I changed how we prevent machine-specific paths from creeping into the specs. Instead of having a “specs mode” env var change app behavior, the specs now scrub the paths from the generated output. This both cleans up app code & makes the specs more realistic. That’s what the <TMP> in the spec output is about.

@DangerCI
Copy link

DangerCI commented Jun 23, 2016

      <tr>
    <td>:white_check_mark:</td>
    <td data-sticky="true"><del><p>Please include a CHANGELOG entry. 

You can find it at CHANGELOG.md.






        :white_check_mark: Good on 'ya.
    
  </th>
 </tr>

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

      <tr>
    <td>:white_check_mark:</td>
    <td data-sticky="true"><del>Note, we hard-wrap at 80 chars and use 2 spaces after the last line.</del></td>
  </tr>
        :white_check_mark: Well done.
    
  </th>
 </tr>

Generated by 🚫 danger

@jpsim
Copy link
Collaborator

jpsim commented Jun 23, 2016

Yay, @pcantrell's back! 😉

On the whole, this looks great. 👍 whenever you get CI passing 😄

@pcantrell
Copy link
Collaborator Author

Nice to be back!

OK, CI’s all sorted. Looks like it was just a Ruby 2.0 vs 2.3 thing.

Before I merge, any opinion on having the separate symbol_qualified key vs. just changing the value of symbol? i.e. this, which is what’s in the PR now:

{
  "file": "/Users/paul/work/bustout/siesta/Source/Error.swift",
  "line": 200,
  "symbol": "transformer",
  "symbol_qualified": "Error.Cause.TransformerReturnedNil.transformer",
  "symbol_kind": "source.lang.swift.decl.var.instance",
  "warning": "undocumented"
}

vs this, which is a (small) breaking change:

{
  "file": "/Users/paul/work/bustout/siesta/Source/Error.swift",
  "line": 200,
  "symbol": "Error.Cause.TransformerReturnedNil.transformer",
  "symbol_kind": "source.lang.swift.decl.var.instance",
  "warning": "undocumented"
}

@jpsim
Copy link
Collaborator

jpsim commented Jun 27, 2016

I agree on changing symbol_qualified to symbol.

@jpsim
Copy link
Collaborator

jpsim commented Jun 27, 2016

👍 feel free to merge this whenever you're ready @pcantrell!

@pcantrell
Copy link
Collaborator Author

Thanks, JP. Will merge when CI passes.

(How can there be no “twiddling thumbs” emoji? …Wait! Aha! ䷄ U+4DC4 = “hexagram for waiting”)

@jpsim
Copy link
Collaborator

jpsim commented Jun 27, 2016

䷄ U+4DC4

Catchy!

@pcantrell
Copy link
Collaborator Author

䷄䷄䷄䷄䷄䷄䷄䷄䷄䷄䷄

Travis during US business hours….

@pcantrell pcantrell merged commit 90313cc into realm:master Jun 27, 2016
@pcantrell pcantrell deleted the improved-lint-report-readability branch June 27, 2016 23:20
@pigeondotdev pigeondotdev modified the milestone: The Past Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants