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

Prepare v0.4 release #339

Merged
merged 99 commits into from
May 12, 2024
Merged

Prepare v0.4 release #339

merged 99 commits into from
May 12, 2024

Conversation

formatc1702
Copy link
Collaborator

As proposed, let's release v0.4 before integrating the large PR #251 into the dev branch.

This PR is intended to do a final check before release, to address any inconsistencies.
The obvious one would be to add the release date to the changelog.

formatc1702 and others added 30 commits October 11, 2021 22:08
`.gv` and `.html` files include the version number as a comment.
Rebuild to avoid diffs during development
Moved metadata and options info further down, so that the core functionality (connectors, cables, connection sets) comes first.
- Use pin names instead of pin indices, until the last moment when generating the ports for the GraphViz nodes
- `Harness.add_mate_pin()` now uses pin names
- Remove unused `if is_arrow()` check from `Harness.connect()`
- Consolidate calling of `Connector.activate_pin()` to prevent subtle bugs
  - Call it from `connect()` and `add_mate_pin()`
  - No longer call it from `create_graph()`
- Misc. other tuning
Co-authored-by: kvid <[email protected]>
Experiments in exporting PDF using `wkhtmltopdf` utility caused borders to disappear when set to 0.25mm, but 0.35mm renders fine
Re-add `parse_file()` for building examples

bla
@formatc1702 formatc1702 mentioned this pull request May 5, 2024
@formatc1702 formatc1702 requested a review from kvid May 5, 2024 13:34
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.

See my suggested changes after looking through the major changes . One of them seemed to be lost, so I had to write it again. I hope I didn't loose any other comments.

docs/CHANGELOG.md Outdated Show resolved Hide resolved
docs/CHANGELOG.md Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/wireviz/wv_colors.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Show resolved Hide resolved
src/wireviz/wv_colors.py Outdated Show resolved Hide resolved
@kvid kvid mentioned this pull request May 6, 2024
@kvid
Copy link
Collaborator

kvid commented May 6, 2024

See my bottom question in #309 (comment) where I point out that the code to be released now only lists quite old Python versions in setup.py and our github workflow. However, I fully understand that you want to avoid including new Python versions in the last minute unless you already have experience using this codebase in that Python version and are confident it has been working for a while with different use cases.

@kvid
Copy link
Collaborator

kvid commented May 8, 2024

After thinking a bit, I now believe it's best to leave the supported Python versions unchanged for this release. #251 has already such changes, and we'll get more time testing it after that is merged into dev. The workflow already test 3.8, and all later minor versions should normally be backwards compatible. I think we therefore can expect them to work with this release as well.

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.

I assume removing the -h CLI option is not intentional.

src/wireviz/wv_cli.py Outdated Show resolved Hide resolved
docs/CHANGELOG.md Show resolved Hide resolved
docs/README.md Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
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.

I suggest expanding the "Installing the development version" section of README to include recommending venv by, e.g. copying commands from #251 (comment) (checking out the dev branch) and also include pip3 install isort and pip3 install black after activating .venv. It will lower the threshold for potential contributors to have a list of commands to get started.
I don't know if it makes sense to create a requirements-dev.txt like this to simplify the README description:

-r requirements.txt
isort
black

Edit: This task about installation description can of course wait unless you have a sequence of commands ready that you have used yourself and don't need testing.

src/wireviz/wv_colors.py Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

formatc1702 commented May 9, 2024

Thank you for your review, I have integrated most suggestions and written a comment on the ones that I didn't.

After thinking a bit, I now believe it's best to leave the supported Python versions unchanged for this release.

Agreed.

I suggest expanding the "Installing the development version" section of README [...] It will lower the threshold for potential contributors to have a list of commands to get started.

Good idea, but let's do this after the refactor is merged into dev, to avoid PRs based on the v0.4 code i.e. on the non-refactored code. A PR to update the README, using the refactored branch as a base, is no problem.

Once v0.4 is merged into master and published, I will try to cherry-pick all of your code suggestions (as far as they are still applicable) to the refactored code so we don't suffer a regression there.

Since most of the current suggestions are quite minor, would you give the OK for releasing v0.4? Then all further review/suggestion effort can be focused on the newer code. Thanks!

formatc1702 and others added 2 commits May 9, 2024 14:10
Remove the references in the CLI help, but keep the placeholders elsewhere in the code as a TODO
kvid
kvid previously approved these changes May 9, 2024
@kvid
Copy link
Collaborator

kvid commented May 9, 2024

Thank's for accepting most of my suggestions.

Note that I pushed a small commit that you should approve or undo. I think it'll help users with old YAML files to get such a notice instead of something that looks like an internal error.

It's also easy to expand this later when more attributes get obsolete or changed.

@formatc1702
Copy link
Collaborator Author

It's a good idea. Let's see if this can be maintained/updated for future changes; for now it should at least catch the big issues, which is already helpful!

@formatc1702
Copy link
Collaborator Author

I am traveling and will be able to merge/publish next week.

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.

I just discovered two of the new examples have "0 m" as cable quantity in the BOM because of missing cable length attributes. If this is an unintentional error, it's very easy to correct before the release, but It's no big deal to me if you intentionally leave it as it is.

Maybe a waning about each zero cable/bundle length in the harness could be a future feature?

examples/ex11.yml Show resolved Hide resolved
examples/ex13.yml Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

Let's leave the examples as they are.
For the future, not specifying any length could indeed trigger a warning, and at the same time, the BOM entry should not list 0m, but simply stay blank.

@formatc1702 formatc1702 merged commit 954c4f5 into master May 12, 2024
4 checks passed
@formatc1702 formatc1702 deleted the release/v0.4-rc2 branch May 12, 2024 11:37
@formatc1702
Copy link
Collaborator Author

v0.4 is released 🎉 The next step is somehow integrating #251 into dev...

@kvid
Copy link
Collaborator

kvid commented May 12, 2024

@formatc1702 wrote:

v0.4 is released 🎉

Congratulations! Thank's a lot for your effort.

The next step is somehow integrating #251 into dev...

First, we need to include all the latest commits from the release into dev. Do you prefer merging master into dev or rebasing dev on top of master? In the latter alternative, do we risk auto-closing other PRs due to force-pushing their base branch?

After that step, we should rebase the #251 branch on top of the updated dev.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented May 17, 2024

Another alternative would be to cherry pick some changes that went into master but not into dev (such as your latest suggestions), or simply adding the refactored equivalent of the change by hand.

I don't think a rebase/merge makes sense since a lot of the changes are not 1:1 applicable to the refactored code anyway. This way, we avoid weird cross-merges as well.

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.

5 participants