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.

Sign up for GitHub

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

Jump to bottom

Analyze/test analysis-options page code excerpts #1912

Merged
merged 5 commits into from
Sep 19, 2019
Merged

Analyze/test analysis-options page code excerpts #1912

merged 5 commits into from
Sep 19, 2019

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Sep 17, 2019 •
edited
Loading

Resolves the second item of #1803.

Staged at https://dart-dev-staging-1.firebaseapp.com/guides/language/analysis-options
Original https://dart.dev/guides/language/analysis-options

I've checked external links:

$ blc -fgi http://localhost:4000/guides/language/analysis-options
Getting links from: http://localhost:4000/guides/language/analysis-options
├───OK─── https://dartpad.dev/
├───OK─── http://dart-lang.github.io/linter/lints/
├───OK─── http://www.google.com/intl/en/policies/privacy/
├───OK─── https://htmlpreview.github.io/?https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/doc/api.html
├───OK─── https://pub.dev/
├───OK─── https://flutter.dev/
├───OK─── http://creativecommons.org/licenses/by/3.0/
├───OK─── https://api.dart.dev/
├───OK─── https://flutter.dev/docs/testing/debugging#the-dart-analyzer
├───OK─── https://pub.dev/packages/analyzer
├───OK─── https://github.com/dart-lang/site-www/tree/master/src/_guides/language/analysis-options.md
├───OK─── https://pub.dev/packages/pedantic
├───OK─── https://github.com/dart-lang/site-www/issues/new?title=%27Customizing%20static%20analysis%27%20page%20issue&body=%0APage%20URL:%20https://dartlang-org-dev.firebaseapp.com/guides/language/analysis-options%0D%0A%0APage%20source:%20https://github.com/dart-lang/site-www/tree/master/src/_guides/language/analysis-options.md%0D%0A%0A%0D%0A%0AFound%20a%20typo?%20You%20can%20fix%20it%20yourself%20by%20going%20to%20the%20page%20source%20and%20clicking%20the%20pencil%20icon.%20Or%20finish%20creating%20this%20issue.%0D%0A%0A%0D%0A%0ADescription%20of%20issue:
├───OK─── https://medium.com/dartlang/announcing-dart-2-5-super-charged-development-328822024970
├───OK─── https://pub.dev/packages/glob
├───OK─── https://medium.com/dartlang
├─BROKEN─ https://pub.dev/documentation/analyzer/latest/analyzer/TodoCode/TODO-constant.html (HTTP_404)
├───OK─── https://github.com/dart-lang/sdk/blob/master/pkg/analyzer/lib/error/error.dart
├─BROKEN─ https://pub.dev/documentation/analyzer/latest/analyzer/AnalysisOptionsWarningCode/ANALYSIS_OPTION_DEPRECATED-constant.html (HTTP_404)
├─BROKEN─ https://pub.dev/documentation/analyzer/latest/analyzer/StaticTypeWarningCode/INVALID_ASSIGNMENT-constant.html (HTTP_404)
├───OK─── https://github.com/dart-lang/linter#linter-for-dart
├───OK─── https://github.com/dart-lang/site-www

The broken links are due to the fact that the analyzer documentation generation failed for the latest release:

Version Uploaded Documentation Archive
0.38.2 Aug 28, 2019 failed  

@chalin chalin added fix.examples Adds or changes example test.general Relates to unit, integration, perf testing labels Sep 17, 2019
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Sep 17, 2019
This was referenced Sep 17, 2019
Closed
Closed
kwalrath
kwalrath approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I have a few small comments.

I'd like an engineer (@filiph or @johnpryan) to look at these changes, too, since they involve more code than I'm comfortable reviewing.

src/_guides/language/analysis-options.md

Here's a sample analysis options file:

{% prettify yaml %}
<?code-excerpt "analysis_options.yaml" from="include" remove="implicit-dynamic" retain="/^$|\w+:|- camel/" remove="http:"?>
```yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... the formatting for yaml got much less vibrant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown code blocks are processed statically and rendered using the highlight package styles (as opposed to the prettifier JS package). I haven't yet tweaked highlight package styles in support of YAML. If you agree that this isn't gating, I'd suggest addressing it in a followup PR (though, if I have time before Filip or John review it, I ca add the style changes to this PR too).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not gating. I'd noticed the difference in styles for awhile, but it's starting to annoy me more and more. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup issue: #1918

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed by #1924

src/_guides/language/analysis-options.md Show resolved Hide resolved
src/_guides/language/analysis-options.md Outdated Show resolved Hide resolved
kwalrath
kwalrath reviewed Sep 18, 2019
View reviewed changes
src/_guides/language/analysis-options.md Outdated
{:.console-output}
<?code-excerpt "analysis/analyzer-results.txt" retain="empty_statements" replace="/ at (lib|test)\/\w+\.dart:\d+:\d+/ at \/*1*\//g"?>
```nocode
lint • Avoid empty statements at /*1*/ • empty_statements
Copy link
Contributor

@kwalrath kwalrath Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part seems weird to me... you'd never see comment text (/*1*/) in there. Can we be more realistic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll see if I can think of something. I have an idea; I'll try it out tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added line numbers and dash-underlined in red just like, say, VS Code does. Let me know if that works for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates done and staging server refreshed. PTAL @kwalrath

johnpryan
johnpryan approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@johnpryan johnpryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@chalin chalin mentioned this pull request Sep 18, 2019
Closed
@chalin
Copy link
Contributor Author

chalin commented Sep 18, 2019

@kwalrath - I justed added the IDE-like analyzer-error highlighting to the implicit-casts example too. PTAL. It is in a separate commit, so I can easily take it out if you don't like it.

Here is what the error highlighting looks like for the two opening examples:

Screen Shot 2019-09-18 at 16 08 56

And here is what it looks like for the implicit-casts example:

Screen Shot 2019-09-18 at 16 10 36

@kwalrath
Copy link
Contributor

Looks great! Feel free to squash and merge this. (I didn't because I wasn't sure if the order of these commits was important.)

@chalin chalin merged commit 9aa9d3b into dart-lang:master Sep 19, 2019
@chalin chalin deleted the chalin-analysis-options-0916 branch September 19, 2019 11:00
@chalin chalin mentioned this pull request Sep 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwalrath kwalrath kwalrath approved these changes

@johnpryan johnpryan johnpryan approved these changes

Assignees
No one assigned
Labels
cla: yes Contributor has signed the Contributor License Agreement fix.examples Adds or changes example test.general Relates to unit, integration, perf testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chalin @kwalrath @johnpryan @googlebot