Skip to content

CP-1140 Initial implementation of failing on analyzer hints #102

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
Merged
merged 1 commit into from
Nov 24, 2015
Merged

CP-1140 Initial implementation of failing on analyzer hints #102

merged 1 commit into from
Nov 24, 2015

Conversation

@travissanderson-wf
Copy link
Contributor

@travissanderson-wf travissanderson-wf commented Nov 21, 2015

travissanderson-wf
travissanderson-wf reviewed Nov 21, 2015
View reviewed changes
lib/src/tasks/analyze/cli.dart Outdated
Copy link
Contributor Author

@travissanderson-wf travissanderson-wf Nov 21, 2015

Choose a reason for hiding this comment

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

might be better to implement hints as an option instead of a flag with values of 'none', 'enabled', 'fatal'?

@codecov-io
Copy link

codecov-io commented Nov 21, 2015

Current coverage is 45.12%

Merging #102 into master will not affect coverage as of 475340a

Powered by Codecov. Updated on successful CI builds.

travissanderson-wf
travissanderson-wf reviewed Nov 23, 2015
View reviewed changes
lib/src/tasks/analyze/cli.dart Outdated
Copy link
Contributor Author

@travissanderson-wf travissanderson-wf Nov 23, 2015

Choose a reason for hiding this comment

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

might be better to implement hints as an option instead of a flag with values of 'none', 'enabled', 'fatal'?

Copy link
Contributor

@evanweible-wf evanweible-wf Nov 23, 2015

Choose a reason for hiding this comment

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

I'm torn on this. I think I like that idea, but it could be confusing to make --hints function differently than the dartanalyzer.

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Nov 23, 2015

+1

1 similar comment
@maxwellpeterson-wf
Copy link
Member

maxwellpeterson-wf commented Nov 23, 2015

+1

@trentgrover-wf
Copy link
Contributor

trentgrover-wf commented Nov 24, 2015

+1
@jayudey-wf ready for merge

@jayudey-wf jayudey-wf changed the title Initial implementation of failing on analyzer hints CP-1140 Initial implementation of failing on analyzer hints Nov 24, 2015
@jayudey-wf
Copy link
Contributor

jayudey-wf commented Nov 24, 2015

@travissanderson-wf can you update the readme with this change as well potentially provide an integration test (@evanweible-wf your thoughts on the latter) ?

@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

There was no existing coverage over AnalyzeTask so I didn't add any

@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

@jayudey-wf added the README info!

@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

I misunderstood the analyze_test - I will add coverage!

@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

@evanweible-wf @maxwellpeterson-wf @trentgrover-wf @jayudey-wf ready for review again

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Nov 24, 2015

Code looks good, +1 assuming CI passes. At that point, I think this is ready for a rebase against latest master and a squash into a single commit.

@travissanderson-wf travissanderson-wf force-pushed the fatal_hints branch 2 times, most recently from f6feeaa to 7cfa4ff Compare November 24, 2015 17:06
@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

@evanweible-wf done

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Nov 24, 2015

+1

1 similar comment
@maxwellpeterson-wf
Copy link
Member

maxwellpeterson-wf commented Nov 24, 2015

+1

@trentgrover-wf
Copy link
Contributor

trentgrover-wf commented Nov 24, 2015

+1
@jayudey-wf ready for merge

@evanweible-wf
Copy link
Contributor

evanweible-wf commented Nov 24, 2015

@travissanderson-wf needs a ddev format 😦

@travissanderson-wf
Copy link
Contributor Author

travissanderson-wf commented Nov 24, 2015

sigh... done

@trentgrover-wf
Copy link
Contributor

trentgrover-wf commented Nov 24, 2015

🎉 +1

@jayudey-wf
Copy link
Contributor

jayudey-wf commented Nov 24, 2015

QA Resource Approval: +10

  • Testing instruction (derived from issue)
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • tests cover new functionality
  • Unit test created/updated
  • All unit tests pass

Merging into master.

jayudey-wf added a commit that referenced this pull request Nov 24, 2015
@jayudey-wf
Merge pull request #102 from travissanderson-wf/fatal_hints
d89d105 
CP-1140 Initial implementation of failing on analyzer hints
@jayudey-wf jayudey-wf merged commit d89d105 into Workiva:master Nov 24, 2015
@jayudey-wf jayudey-wf mentioned this pull request Nov 25, 2015
Merged
@evanweible-wf evanweible-wf mentioned this pull request Dec 7, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@travissanderson-wf @codecov-io @evanweible-wf @maxwellpeterson-wf @trentgrover-wf @jayudey-wf