Skip to content

Conversation

@skshetry
Copy link
Collaborator

Closes #8956.

@skshetry skshetry requested a review from efiop February 15, 2023 10:51
Comment on lines +49 to +52
q = colorize("Having any troubles?", "yellow")
link = colorize("https://dvc.org/support", "blue")
footer = f"\n{q} Hit us up at {link}, we are always happy to help!"
ui.error_write(footer)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tbh I'd prefer getting rid of this completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

@skshetry skshetry Feb 15, 2023

Choose a reason for hiding this comment

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

  • It's noisy
  • not all errors are critical or require support. Errors are part of the workflow for the most part. eg:
$ dvc import-url https://data.dvc.org/get-started/data.xml

ERROR: unexpected error - [Errno 17] File exists: 'data.xml'

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!
  • we don't have a way to distinguish if those errors are internal error or just normal UX/workflow. Even more so with fsspec, since they raise OSError. Even if we could, dvc's scope has increased so much that it's hard to keep this distinction.
  • unnecessary support link/message gets added to the output

Also see #4126 and #7018.

Copy link
Contributor

@daavoo daavoo Feb 16, 2023

Choose a reason for hiding this comment

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

Thanks for the details @skshetry .
It makes sense, but feels like most of the reasons are not really about the message itself but the circumstances around it.

I believe it would still be useful if we could show it when it really should.

I would even try to extend it by automatically dumping the traceback or something and making it easy to share (since that's basically what we already ask when someone reports an error from the support channel).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add a config to opt-out to begin with

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, but atm the reality is that it's not helpful. It's not clear to me if that link is helpful to the users.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but atm the reality is that it's not helpful. It's not clear to me if that link is helpful to the users.

IIn the current state, I am ok with dropping it

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 93.07% // Head: 93.06% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (53865c8) compared to base (6f87c14).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9034      +/-   ##
==========================================
- Coverage   93.07%   93.06%   -0.02%     
==========================================
  Files         456      456              
  Lines       36814    36761      -53     
  Branches     5335     5324      -11     
==========================================
- Hits        34264    34210      -54     
- Misses       2025     2026       +1     
  Partials      525      525              
Impacted Files Coverage Δ
dvc/logger.py 85.71% <ø> (-0.11%) ⬇️
dvc/cli/__init__.py 80.62% <100.00%> (+0.62%) ⬆️
dvc/repo/diff.py 95.31% <0.00%> (-3.00%) ⬇️
dvc/render/convert.py 87.09% <0.00%> (-0.41%) ⬇️
dvc/output.py 89.18% <0.00%> (-0.30%) ⬇️
dvc/render/converter/vega.py 91.20% <0.00%> (-0.05%) ⬇️
tests/func/test_diff.py 100.00% <0.00%> (ø)
dvc/repo/index.py 89.88% <0.00%> (+0.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@skshetry skshetry merged commit ddeb32e into treeverse:main Feb 16, 2023
@skshetry skshetry deleted the footer-in-errstream branch February 16, 2023 05:46
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.

Bash completion for DVC includes stylized help text

2 participants