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: command error as comment #502

Merged
merged 20 commits into from
Jan 13, 2023
Merged

Conversation

zolamk
Copy link
Contributor

@zolamk zolamk commented Dec 5, 2022

Description

Currently, a command might be syntactically correct but during execution of the generated program for a command an error might happen which makes it seem like it's a reviewpad error when in a reality it was the command that failed because of wrong input so this PR provides feedback to the user in the form of a GitHub comment.

Related issue

Closes #471

Type of change

Improvements (non-breaking change without functionality)

How was this tested?

Unit tests and Manual tests

Checklist

  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • I have ran task check -f and have no issues

Code review and merge strategy (ship/show/ask)

  • Ask: this pull request requires a code review before merge

@reviewpad reviewpad bot added ask Pull request requires a code review before merge run-build Label that controls when build should be executed critical Modifications to critical changes labels Dec 5, 2022
@reviewpad reviewpad bot requested a review from marcelosousa December 5, 2022 07:10
@reviewpad reviewpad bot added the external-contribution External contribution label Dec 5, 2022
@reviewpad reviewpad bot added the plugins Modifications to the plugins directory label Dec 5, 2022
@reviewpad reviewpad bot requested a review from shay2025 December 5, 2022 07:10
@reviewpad reviewpad bot added the large large pull request label Dec 5, 2022
@reviewpad reviewpad deleted a comment from reviewpad bot Dec 6, 2022
@ferreiratiago ferreiratiago added the waiting-review PR waiting for review label Dec 20, 2022
@ferreiratiago
Copy link
Member

This pull request is on hold due to low priority.

@reviewpad
Copy link

reviewpad bot commented Dec 20, 2022

Reviewpad Report

⚠️ Warnings

  • The pull request does not have a linear history

‼️ Errors

  • Unconventional commit detected: 'Merge branch 'main' into feat/command-error-as-comment' (4880477)

ℹ️ Messages

  • @marcelosousa: you are being notified because critical code was modified

@ferreiratiago ferreiratiago added this to the v3.20.0 milestone Dec 20, 2022
@ferreiratiago ferreiratiago modified the milestones: v3.20.0, v3.21.0 Dec 29, 2022
Copy link
Member

@ferreiratiago ferreiratiago left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Please have a look at the comment 🙏

@@ -51,6 +52,7 @@ type BaseEnv struct {
RegisterMap RegisterMap
Report *Report
Target codehost.Target
EventData *handler.EventData
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need EventData? EventPayload should be enough for what we need here. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes perfect sense, thank you

@@ -9,7 +9,8 @@ type Statement struct {
}

type Program struct {
statements []*Statement
IsFromCommand bool
Copy link
Member

Choose a reason for hiding this comment

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

IsFromCommand should be another property of the env in which the program is running. I would suggest moving this into the BaseEnv. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one I think better to keep it in the program since we are checking if it's a command after the BaseEnv is built I think, what do you think @ferreiratiago

@reviewpad reviewpad bot added the requires-author-attention Pull request requires the author to take action label Jan 2, 2023
@reviewpad reviewpad bot requested a review from ferreiratiago January 3, 2023 12:26
@ferreiratiago ferreiratiago modified the milestones: v3.21.0, v3.22.0 Jan 9, 2023
@ferreiratiago ferreiratiago merged commit b5b3a72 into main Jan 13, 2023
@ferreiratiago ferreiratiago deleted the feat/command-error-as-comment branch January 13, 2023 18:19
@reviewpad
Copy link

reviewpad bot commented Jan 13, 2023

📈 Pull Request Metrics

💻 Coding Time: 1 day
🛻 Pickup Time: 28 days
👀 Review Time: 11 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ask Pull request requires a code review before merge critical Modifications to critical changes external-contribution External contribution large large pull request low priority plugins Modifications to the plugins directory requires-author-attention Pull request requires the author to take action run-build Label that controls when build should be executed waiting-review PR waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide feedback as a comment when command fails
3 participants