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

feat: build failure message from invalid hunks #90

Merged
merged 5 commits into from Aug 21, 2020

Conversation

chingor13
Copy link
Contributor

This is a barebones implementation that we can start with to build a message for hunks that do not match the PR diff (so they cannot be suggested via review comments).

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (comment-pr@13908f8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             comment-pr      #90   +/-   ##
=============================================
  Coverage              ?   86.17%           
=============================================
  Files                 ?       22           
  Lines                 ?     1938           
  Branches              ?      143           
=============================================
  Hits                  ?     1670           
  Misses                ?      267           
  Partials              ?        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13908f8...b1403e6. Read the comment docs.

@chingor13 chingor13 marked this pull request as ready for review August 21, 2020 21:00
@chingor13 chingor13 requested a review from a team August 21, 2020 21:00
@chingor13 chingor13 requested a review from a team as a code owner August 21, 2020 21:00
Copy link
Contributor

@TomKristie TomKristie left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

* @param invalidHunks a map of filename to hunks that are not suggestable
*/
export function buildErrorMessage(invalidHunks: Map<string, Hunk[]>): string {
if (invalidHunks.size === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why return an empty string? if there is no error message to be made there should be no message.
You can make the return type : string | null to return null

Copy link
Contributor Author

@chingor13 chingor13 Aug 21, 2020

Choose a reason for hiding this comment

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

I'm not sure how this was going to be used. I'd assume we actually want this check outside this and handle that case in the caller.

I chose to return an empty string to be more confident about types in what's being returned. I could note in the description that we will return empty string for an empty input but 🤷. If you were going to check if the result of this was empty or null, you might as well check that the input is empty before calling this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup that works

@chingor13 chingor13 merged commit ecdb615 into googleapis:comment-pr Aug 21, 2020
chingor13 added a commit that referenced this pull request Sep 25, 2020
…n pull requests (#105)

* feat(patch text to hunk bounds): support regex for patch texts (#83)

* fix(patch text to hunk bounds): support regex for patch texts

* more comments and more tests

* fix(framework-core): core-library get remote patch ranges (#84)

* fix(framework-core): given files old content and new content, compute the valid hunks (#86)

* fix(framework-core): parse raw changes to ranges

* refactor(framework-core): rename modules, functions, & re-org project structure (#89)

* fix(framework-core): hunk to patch object (#91)

* feat: build failure message from invalid hunks (#90)

* test: add failing stub and test for building the failure message

* fix: implement message building

* fix: use original line numbers in error message

* docs: add docstring

* docs: add note about empty input returning empty string

* feat(framework-core): comment on prs given suggestions (#93)

* feat(framework-core): main interface for create review on a pull request (#114)

* feat(framework-core): main interface for create review on a pull request

* docs: fix typo

* nits and typos...

* gts lint warning fix

* fix(framework-core): combine review comments (#116)

* fix(framework-core): collapsing timeline and inline comments into single review

* test: fixed imports

* added case when there are out of scope suggestions and no valid suggestions

* feat(framework-core): return review number and variable renaming (#117)

* feat(framework-core): return review number and variable renaming

* lint

Co-authored-by: Jeff Ching <chingor@google.com>
Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>
Co-authored-by: Benjamin E. Coe <bencoe@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants