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(cli): CAN-340 Show step name on error #1097

Closed
wants to merge 1 commit into from

Conversation

noahlitvin
Copy link
Contributor

Needs more testing/verification

Copy link

changeset-bot bot commented Jun 4, 2024

⚠️ No Changeset found

Latest commit: 6a833ee

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@noahlitvin noahlitvin changed the title CAN-340 feat(cli): CAN-340 Show step name on error Jun 4, 2024
Copy link

nx-cloud bot commented Jun 4, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6a833ee. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@noahlitvin noahlitvin changed the base branch from main to fix-all-artifacts-for-trace June 18, 2024 01:43
@noahlitvin noahlitvin mentioned this pull request Jun 18, 2024
@dbeal-eth dbeal-eth force-pushed the fix-all-artifacts-for-trace branch from 3a9f017 to d85093f Compare June 25, 2024 05:34
Base automatically changed from fix-all-artifacts-for-trace to main June 25, 2024 05:41
@mjlescano
Copy link
Contributor

mjlescano commented Jun 26, 2024

Mmm, tried it out for example putting an erroneous contract name but in both cases, on main and this branch I get the step name:

Screenshot 2024-06-26 at 22 30 39 Screenshot 2024-06-26 at 22 30 54

Maybe there's another use case that I'm not trying?

@noahlitvin
Copy link
Contributor Author

Yea, I think the improvement is surfaced if e.g. it throws an error parsing a template string? (Like see what follows the log output when it's in the new position from this pr)

@mjlescano
Copy link
Contributor

mjlescano commented Jul 5, 2024

This PR showed no difference on these three cases (first two we can still see the name on main):

  1. When doing something invalid on templating:
Screenshot 2024-07-05 at 14 44 42
  1. When throwing an error on constructor:
Screenshot 2024-07-05 at 14 47 48
  1. When adding an invalid depends, this case didn't show the step name on any branch:
Screenshot 2024-07-05 at 14 50 16

Case 3. is the one we should be fixing probably, maybe am i missing some other case?

Copy link
Contributor

@dbeal-eth dbeal-eth left a comment

Choose a reason for hiding this comment

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

the problem with this implementation is that it prints out the step name every single time, even if its not actually being executed

in order to determine whether a step should be executed or not, its configuration needs to be computed, which requires doing some things which may result in error.

I think the only approach we have to this is adding a fake log message for this if an error is generated and only in that case.

@noahlitvin noahlitvin closed this Jul 5, 2024
@mjlescano mjlescano deleted the show-step-on-error branch July 7, 2024 11:04
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

3 participants