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

Error "first item not found anywhere" is unclear does not indicate the name of the item #207

Open
hyphz opened this issue Dec 27, 2020 · 7 comments

Comments

@hyphz
Copy link

hyphz commented Dec 27, 2020

It would be very helpful to list the name of the item that is not found, and the meaning of the error message is not clear at the moment (the first item of what?).

@kvid
Copy link
Collaborator

kvid commented Dec 28, 2020

Thank you for reporting this unclear error message. When you report something about what happens when running WireWiz, it will be much easier for others to reproduce the same state if you include more information when submitting an issue.

However, in your case I'm able to reproduce the same error message using e.g. this YAML input that contains a connection to non-existing connectors/cables:

connections:
  - - X: 1
    - Y: 1

In a more realistic scenario, this error can easily happen when misspelling a connector/cable name. I agree that it will help the user to know the name of the item that is not found, and that this name is expected to be found in the set of connectors or cables. The exception is raised here: https://github.com/formatc1702/WireViz/blob/606ddbf9775da6300299640501083acfaef42077/src/wireviz/wireviz.py#L91

I suggest changing this to something like:

            raise Exception(f'Connection {cindex} item {first_item} is not in neither connectors nor cables')

Where cindex is a 1-based index into the list of connections - obtained by modifying the connection loop: https://github.com/formatc1702/WireViz/blob/da568412908c8daae7e57565ce446e81fd0140a5/src/wireviz/wireviz.py#L74
Replace the loop code line above with:

    for cindex, connection in enumerate(yaml_data['connections'], start=1):

There are also several other exceptions in WireViz that need improvements like this. Maybe we should raise a more general issue to identify more of them?

@formatc1702
Copy link
Collaborator

formatc1702 commented Dec 29, 2020

There are also several other exceptions in WireViz that need improvements like this. Maybe we should raise a more general issue to identify more of them?

Yes.

More user-friendly, helpful, and standardized (in terms of grammar) error messages have been on my mind for a while.

If @hyphz doesn't mind, I will hijack this issue and generalize as @kvid as suggested. Thanks!

Edit: The YAML parsing is being completely refactored in #186, so any attempts to improve the code should start from that branch, or wait until that PR is merged into dev.

@martinrieder
Copy link
Contributor

martinrieder commented May 16, 2024

If there was a way to have a machine-readable link of the error output to the YAML input, this would highly improve the usability in nanangp/vscode-wireviz-preview

@nanangp see also #305 (comment)

@kvid
Copy link
Collaborator

kvid commented May 17, 2024

In PR #251 I've replaced some print() warnings with warnings.warn(), and I suggest we could do that all over.

@martinrieder
Copy link
Contributor

Sure. That will definitely help, but still not refer to the actual line number from the YAML input. Therefore, I am linking this to the parser replacement issue: #223 (comment)

@martinrieder
Copy link
Contributor

martinrieder commented Jun 9, 2024

@kvid wrote:

In PR #251 I've replaced some print() warnings with warnings.warn(), and I suggest we could do that all over.

Please take a look at Structlog. It can act as a drop-in replacement, while offering full configurability. See the demonstration of what an exception could look like:
.png

In combination with this, you could even output logs in machine readable format using YAML:

@kvid
Copy link
Collaborator

kvid commented Oct 19, 2024

To avoid errors like #426 shows, we should raise a TypeError if detecting any non-dict connector or cable entry, by e.g. insert something like this initially in the body of the loop to parse such entries from the YAML input file:

if not isinstance(attribs, dict):
    raise TypeError(f"Expecting a dict value for '{key}' in '{sec}', but got {type(attribs)}:\n{attribs}")

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

No branches or pull requests

4 participants