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
added issue reference to todos #3299
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@kwalrath please review my PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @vendz!
I believe that the test failures are due to line length > 80. Could you break up the lines? While you're at it, you might make these full URLs. Or not. :)
@kwalrath I have made changes to the TODO line so if you could review my PR now |
@parlough i have made the changes you requested in this PR itself so if you could review it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions, feel free to ignore them if desired.
Thanks again!
// TODO(miquelbeltran) language-tour.md should explain that | ||
// non-required params can be null (#3297) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this one instead like mentioned in #3297 and include Fixes #3297
again?
// TODO(miquelbeltran) language-tour.md should explain that | |
// non-required params can be null (#3297) |
// TODO(chalin): I believe that handleError's test argument should be declared (#3298) | ||
// as follows, but it isn't: {bool Function(dynamic error) test}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you place it after the whole comment and instead put a link to the issue so its easier to access when navigating the source?
// TODO(chalin): I believe that handleError's test argument should be declared (#3298) | |
// as follows, but it isn't: {bool Function(dynamic error) test}. | |
// TODO(chalin): I believe that handleError's test argument should be declared | |
// as follows, but it isn't: {bool Function(dynamic error) test}. | |
// (https://github.com/dart-lang/site-www/issues/3298) |
@parlough made the changes you requested. |
@vendz Thanks for all the work, looks good to me! I'm running the checks and if they're all green, I'll merge these changes :) |
in reference to #2119