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

Use portable OS error codes so program doesn't crash #318

Merged
merged 5 commits into from
Sep 1, 2023

Conversation

RedshiftVelocities
Copy link
Contributor

I think OSError had an update at some point, and the error code the program looks for changed. Anyhow this change fixes the titleblock-added version of the program.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

  1. I suggest using the Standard errno system symbols to avoid numeric constants that differ between platforms.
  2. We might need to test for other errors as well. I managed to get this error when trying an 80000 character dummy string in Python 3.9.13 at Windows 10: ValueError: _getfinalpathname: path too long for Windows
  3. This is a typical problem where it helps a lot to have unit tests for different edge cases to make it easier to execute the same tests at all platforms (often several developers are involved because not all developers have access to all platforms). I suggest we start collecting such test code. Until we get a better structure for test code, I suggest some clearly marked unit test function at the bottom of the source file containing the major part of the code to be tested.
  4. Edit: I just checked this part of the code in PR Large scale refactoring #251 where @formatc1702 has made major changes on top of the latest branch which is the result of joining changes from several other PRs. It seems his new code assumes the string to be YAML source if it contains a newline character, and therefore don't need to check for error numbers. However, I don't know yet when we can expect Large scale refactoring #251 to be merged into the dev branch. First, I expect the latest branch to be merged into the dev branch this summer, but that code still contains the code with numeric constants you try to fix in this PR. If we also manage to finalize Large scale refactoring #251 during the summer, then this PR might get obsolete.

src/wireviz/wireviz.py Outdated Show resolved Hide resolved
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
@RedshiftVelocities
Copy link
Contributor Author

For my own use, is there currently any version of the program that supports both the generation of drawings with a titleblock as well as autogeneration of connectors?

@formatc1702 formatc1702 deleted the branch wireviz:dev June 7, 2023 18:19
@formatc1702 formatc1702 closed this Jun 7, 2023
@kvid
Copy link
Collaborator

kvid commented Jul 16, 2023

@RedshiftVelocities I just found out to my surprise that this PR was closed. I believe it was not intentional, but rather an automatic consequence of deleting the base branch of your PR (this is an unfortunate functionality of github).

This base branch was one of several branches that got deleted when merging their PRs into the dev branch (all PRs from the latest branch - see also #316 for a description of the process). I expect the current dev branch to contain what you need for your own use, but I've not yet been able to test it myself. PR #251 is not yet merged into the dev branch, so your PR is not obsolete.

It might be hard to reopen such an automatically closed PR, but you should be able to rebase your branch on top of the current dev branch, then create a new PR with dev as base branch, and call it a continuation of this PR.

@kvid kvid reopened this Sep 1, 2023
@kvid kvid changed the base branch from feature/technical-drw to dev September 1, 2023 18:59
I think OSError had an update at some point. Anyhow this change fixes this version of the program.
The error code is 22 on windows, apparently
Co-authored-by: kvid <kvid@users.noreply.github.com>
Co-authored-by: kvid <kvid@users.noreply.github.com>
@kvid
Copy link
Collaborator

kvid commented Sep 1, 2023

I managed to re-create the deleted base branch at the exact same commit as it was when deleted earlier this summer. That enabled me to re-open this PR. Then I changed the base branch to dev because all commits from the old base branch has already been merged into dev. More commits have later been added on top of dev, so I rebased your PR branch on top of the current dev to prepare a merge. I'm sorry if force-pushing your branch causes any trouble for your local branch, but I have a backup of the previous state, so It will be possible to undo it if needed.

@kvid kvid changed the title Update error code so program doesn't crash Use portable OS error codes so program doesn't crash Sep 1, 2023
@kvid kvid merged commit 6f9007f into wireviz:dev Sep 1, 2023
2 checks passed
kvid added a commit to kvid/WireViz that referenced this pull request Sep 2, 2023
kvid added a commit to kvid/WireViz that referenced this pull request Sep 2, 2023
martinrieder added a commit to martinrieder/WireViz that referenced this pull request Jul 1, 2024
martinrieder added a commit to martinrieder/WireViz that referenced this pull request Jul 1, 2024
kvid pushed a commit that referenced this pull request Jul 4, 2024
kvid pushed a commit that referenced this pull request Jul 4, 2024
kvid pushed a commit that referenced this pull request Jul 5, 2024
In Windows might ValueError be raised instead of the already
catched exceptions in some cases (depending on the Python version)

Fixes point 2 of #318 (review)
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.

3 participants