search

wireviz / WireViz

Easily document cables and wiring harnesses.
GNU General Public License v3.0
4.41k stars 225 forks source link

[meta] Collaborator discussion #101

Open formatc1702 opened 4 years ago

formatc1702 commented 4 years ago

I've created a new milestone to show which features I'd like to have included in the next release, i.e. pushing to master as well as uploading the new package to PyPI.

If anybody wants to suggest more issues to include, please let me know. However, my aim is to release v0.2 relatively soon, which is why I've deliberately left out a lot of existing requests.

formatc1702 commented 4 years ago

Uptate on the v0.2 milestone: I've assigned people for most open issues/PRs, hoping to be able to assign #114 soon too. If anybody wants to take on #60, please comment there and I will asign you.

Additionally, I've made a milestone for v0.3, which currently only serves to denote issues/PR drafts that I know I want to pursue as soon as v0.2 is out, but not before.

kvid commented 4 years ago

Have you considered #116 and #121 for the v0.2 milestone?

formatc1702 commented 4 years ago

Added both! Thanks.

kvid commented 4 years ago

I just made a suggestion in the closed issue https://github.com/formatc1702/WireViz/issues/122#issuecomment-667545193. Do you think it should be reopened?

formatc1702 commented 4 years ago

Feel free to do so!

kvid commented 4 years ago

Feel free to do so!

I don't know how. Maybe I'm not authorized to do it.

formatc1702 commented 4 years ago

Not sure.. did it myself now :)

Perhaps only repo owners (and maybe the original commenter) have the "reopen issue" button...

Screen Shot 2020-08-01 at 21 12 45
kvid commented 4 years ago

Not sure.. did it myself now :)

Thank's

Perhaps only repo owners (and maybe the original commenter) have the "reopen issue" button...

You are right. I certainly did not see that button here, but I can see it in one of the closed issues I opened myself.

formatc1702 commented 4 years ago

Just a quick FYI especially for @kvid, I'll be taking a couple of days off coding to breathe a bit, and I promise I shall be back to read through all the comments, commits and other contributions as usual. It will help me get back my focus and ensure v0.2 gets released with no major issues :)

formatc1702 commented 4 years ago

My goal is to get v0.2 released by the end of the week.

"Perfect is the enemy of the good", so let's get this release out soon and hopefully receive feedback on the new features. I suggest that after the release, we can decide on prioritizing the upcoming issues to get another cool set of features for v0.3!

Thanks everybody who's contributed so far, with major kudos to @kvid for submitting good PRs and doing some thorough code review!

formatc1702 commented 4 years ago

Unforeseen circumstances prevented me from working on WireViz this week, so the v0.2 release will have to wait another 2-3 weeks until after my holidays 😐 oh well...

formatc1702 commented 4 years ago

Here's a quick update for everyone, especially for @kvid, @Tyler-Ward, @martonmiklos, @zombielinux [and others?] who have submitted contributions while I was gone. Thanks a lot for your continued support, and interest in this project!

I hope to resume reviewing/merging/coding in the upcoming weeks, so don't worry, this project is not forgotten!

Holidays, personal and work related issues have kept me busy and delayed things quite a bit... I will get back to every single issue and PR and integrate them into the project one by one as I find time to keep working on WireViz. I need a few more days to be able to continue, but rest assured, I am looking forward to integrate new features and get new releases out the door! Cheers!

kvid commented 4 years ago

When the PRs are ready and unless rejected by @formatc1702 , I suggest he merge them into dev in this order: #115, #164, #163, #153, and #111, but please give the PR authors a couple of days for rebasing on top of the new dev and resolving conflicts before each merge.

formatc1702 commented 4 years ago

Thanks for the suggestion. I haven't looked at the contributions in detail; just one question:

Why not prioritize #153 and #111, which were planned for v0.2 already, get that one released, and then continue with the other PRs you mention? I'm worried that the v0.2 release will be further delayed if we try to include every new addition, and it might be better to make a clean cut between what goes into v0.2, and what will go into v0.3 and beyond.

If there's a technical reason that would prevent this, please let me know. Thanks!

kvid commented 4 years ago

Thanks for the suggestion. I haven't looked at the contributions in detail; just one question:

Why not prioritize #153 and #111, which were planned for v0.2 already, get that one released, and then continue with the other PRs you mention? I'm worried that the v0.2 release will be further delayed if we try to include every new addition, and it might be better to make a clean cut between what goes into v0.2, and what will go into v0.3 and beyond.

I see your point.

If there's a technical reason that would prevent this, please let me know. Thanks!

formatc1702 commented 4 years ago

Thanks for the detailed answer.

There's tons of new stuff in v0.2 already, compared to the current master branch, so it's nice to keep some good stuff for the following release ;)

stevegt commented 4 years ago

@formatc1702 Let us know how we can help. I'm extremely impressed by how motivated and active the contributors to this project have been in its short life, and I'd like to do what I can to ensure its continuity. I'm already pretty sure the WireViz yaml schema could become a defacto industry standard, though that does raise the thorny issue of governance -- maybe some sort of RFC or PEP-like approach might work. I'm also thinking of suggesting you get on the waiting list for the github Sponsors program if you haven't already.

formatc1702 commented 4 years ago

@formatc1702 Let us know how we can help. I'm extremely impressed by how motivated and active the contributors to this project have been in its short life, and I'd like to do what I can to ensure its continuity. I'm already pretty sure the WireViz yaml schema could become a defacto industry standard, though that does raise the thorny issue of governance -- maybe some sort of RFC or PEP-like approach might work. I'm also thinking of suggesting you get on the waiting list for the github Sponsors program if you haven't already.

Thanks @stevegt for the kind words! It has been amazing to see the project grow, and indeed, collaboration with all the contributors has been great!

I'll lookinto the GitHub Sponsors thing; however, I'm unsure whether monetizing the project in this way might negatively impact users' expectations, and contributors' attitudes, since a lot of WireViz is built on their work, not only mine.

formatc1702 commented 4 years ago

Let us know how we can help.

Since my time for working on the project has been a bit limited lately, the following things have helped immensely:

This is all highly appreciated, and I hope we can keep the momentum of the project going :)

formatc1702 commented 4 years ago

Posting it here as well for completeness sake: v0.2 has been released, thank you everybody for your valuable contributions!

Over 100 forks and >2000 stars on the project really tell me we're working on something worthwhile here!

formatc1702 commented 3 years ago

Short summary of the current situation:

As you can see, I'd like to reduce the number of open PRs first. Afterwards, I'll try and prioritize some issues that deserve attention :-)

kvid commented 3 years ago

Short summary of the current situation:

  • 192 seems to be working, will re-review it soon and merge

  • I am currently working on #186 and hope to have a version that is ready for review soon-ish

  • 181 has been updated to reflect the recently merged PRs

  • I could use some help with the following:

    • Getting #171 ready for merge (high priority, has been waiting for a while)

I have applied the commits from #171 + bugfix onto the current #192 commits and are ready to rebase it on top of dev and create a PR after #192 is merged in.

  • Getting #168 ready for merge

I have done some work on this as well, but need more time because I'm not able to contribute every day.

formatc1702 commented 3 years ago

Thanks. #192 is now merged; looking forward to merging your upcoming PR and closing all the related partial PRs.

amotl commented 3 years ago

Hi @formatc1702, @kvid and @stevegt,

within #211, we just reported about our efforts on bringing WireViz to the Web. It has actually just been a (re)packaging operation of code already conceived within the community (thanks, @elbosso!). However, we hope you will appreciate having this available as a standalone package and we are looking forward to any contributions and comments.

Happy hacking and with kind regards, Andreas.

amotl commented 3 years ago

Hi @formatc1702 and @kvid,

we just recognized that many improvements have been made on behalf of [1] and [2] (#197) already. Those look like valuable improvements to WireViz.

Just let me know when you are ready and I will update wireviz-web accordingly.

With kind regards, Andreas.

[1] https://github.com/formatc1702/WireViz/tree/dev [2] https://github.com/kvid/WireViz/tree/simplify-BOM-code

formatc1702 commented 3 years ago

Sorry for the lack of activity.

Here's the current state of things:

I cannot promise a time frame for this, but I am hopeful that I can start the project moving again in the foreseeable future ;-)

Thanks everyone for your contributions.

formatc1702 commented 3 years ago

@kvid if you could let me know what order the mentioned PRs should be merged, and whether you need to do anything on them, that would help me to integrate them in the most painless way possible :) Thanks.

kvid commented 3 years ago

As #197 and #229 are already merged, #214 and #219 can follow in that order. I have rebased them both on top of the current dev. Please review them (anyone?) to increase the quality before merging. It should be possible to merge #229 both before or after them, but I have not checked for conflicts. I experience a lot of conflicts with import statements while rebasing. I think we should consider splitting imports into several lines (one identifier per line) and sort them alphabetically per import statement to reduce conflicts. Maybe there is a PEP describing this?

kvid commented 3 years ago

I think this project follows PEP8 import guidelines more or less, but I think we should also consider sorting imports according to some common rules to reduce merge conflicts. Maybe a tool like isort can be used to automate the task? Some projects also combine isort with black to force a fully unified code formatting on all Python files. Please tell about your own experiences with formatting rules and tools like this.

formatc1702 commented 3 years ago

It will soon be a year since v0.2 of WireViz was released. There are some rather big PRs pending, but I think that the dev branch has been in a very good shape recently (the only known major bug is #220, and a solution is on the way) so I have decided I will release v0.3 soon as it is in the current state because it includes some important features that I don't want to hold back any longer.

formatc1702 commented 3 years ago

I experience a lot of conflicts with import statements while rebasing. I think we should consider splitting imports into several lines (one identifier per line) and sort them alphabetically per import statement to reduce conflicts. Maybe there is a PEP describing this?

I think we should also consider sorting imports according to some common rules to reduce merge conflicts

Recently, I have tried to stick to the following scheme and am trying to make this consistent across files. Packages sorted alphabetically per block, and individual modules sorted alphabetically within a package's line.

# built-in packages
import os
from pathlib import Path
import sys
# ...
        # blank line
# external dependencies
from graphviz import Graph
import yaml
# ...
        # blank line
# internal modules
from wireviz.DataClasses import Connector, Cable, Metadata, Options, Tweak
from wireviz.wv_colors import get_color_hex, translate_color
# ...

There will surely be some conflicts in the near future, but IMHO it is a clean and structured guideline that should reduce conflicts in the future.

amotl commented 3 years ago

Dear Daniel,

we can warmly recommend using the excellent isort and black programs from the Python ecosystem to bring the codebase into a standards-based shape on those matters.

With kind regards, Andreas.

P.S.: Ah, I just recognized that @kvid already recommended those programs just two posts above, at https://github.com/formatc1702/WireViz/issues/101#issuecomment-904688129. Apologies.

formatc1702 commented 3 years ago

Thanks @kvid and @amotl for the isort and black suggestions! I am trying them out and I have to say, they look really promising! Do you have experience with PRs branching off before reformatting with black, and attempting to merge after reformatting? Is it reasonable to expect that merging will go smoothly if both the base branch and the PR branch are reformatted independently?

amotl commented 3 years ago

Hi again,

I just wanted to leave another note on the isort/black thing. With https://github.com/earthobservations/gribmagic/commit/0d795ea, I just recently added it to another project and found the --profile=black option of isort sensible.

isort --profile=black foobar
black foobar

This option is important to make isort's formatting of imports compatible with Black's code-formatting style - see also [1].

Do you have experience with PRs branching off before reformatting with black, and attempting to merge after reformatting? Is it reasonable to expect that merging will go smoothly if both the base branch and the PR branch are reformatted independently?

If both branches have been formatted with the same options, I believe it is reasonable to expect that merges will apply well without further ado.

With kind regards, Andreas.

[1] https://pycqa.github.io/isort/docs/configuration/black_compatibility.html

formatc1702 commented 3 years ago

WireViz v0.3 is released 🎉

And we already have a bunch of very interesting PRs that could warrant a v0.4 not long after!

formatc1702 commented 3 years ago

I have rebased some of the open PRs on top of each other, in order to be able to have a latest branch that incorporates all the changes, while still keeping each individual PR open for review before inclusion into dev.

I encourage any users wishing to test the features in the following PRs to use the latest branch and try them out!

Bear in mind that this branch might not be as stable as dev. Please report any bugs in the corresponding PR. If in doubt of which of them is causing the issue, you may also open a new issue. Ideally, any code reviews and approvals of the PRs should happen in the order of the list above. This way, the dev branch can catch up with latest step by step.

Due to the changes introduced in #244, a fresh install of WireViz using pip is recommended, since the CLI parsing is now handled by wv_cli.py, and the CLI entry points have therefore moved.

I am sorry for any inconvenience caused by force-pushing the PR branches after rebasing. However, I believe this is the best approach to allow me to continue developing the project freely and on top of all the latest features, without compromising the relative stability of the dev branch. Additionally, every time I run WireViz from latest, I am essentially testing all the above PRs simultaneously, so any bugs can hopefully be caught faster.

I welcome any comments on this new approach.

amotl commented 3 years ago

I welcome any comments on this new approach.

I believe it is a good decision in order to move forward. Thanks a stack for your work and keep up the spirit!

kvid commented 3 years ago

I just noticed a hotfix is considered, as described in issue https://github.com/formatc1702/WireViz/pull/254#issuecomment-950121982. If that will be released, I suggest also adding to the syntax description a simple warning that states all bgcolor_* attributes (and possibly some bgcolor attributes) are experimental and might get a different syntax in future releases. A future merge of #255 will break this syntax, and I recommend a warning about this.

formatc1702 commented 3 years ago

I just noticed a hotfix is considered, as described in issue #254 (comment). If that will be released, I suggest also adding to the syntax description a simple warning that states all bgcolor_* attributes (and possibly some bgcolor attributes) are experimental and might get a different syntax in future releases. A future merge of #255 will break this syntax, and I recommend a warning about this.

There will certainly be some more syntax changes in the future, such as for autogeneration of components (#186), potentially renaming connector.style to connector.category (as propsed here), and maybe others.

There is also the general warning in the README concerning this:

This is very much a work in progress. Source code, API, syntax and functionality may change wildly at any time.

Therefore, I see no need for an additional warning here.

martonmiklos commented 2 years ago

Hi folks,

I am trying to get the #189 working. What is the status of the latest branch?

I got the following error:

mm@mm-precision:~/Dokumentumok/BlackEye/loopback/drawings/WireViz$ wireviz ../h/h.yaml 

WireViz 0.4-dev
Input file:   ../h/h.yaml
Output file:  ../h/h.[html|png|svg|tsv]
Traceback (most recent call last):
  File "/home/mm/.local/bin/wireviz", line 11, in <module>
    load_entry_point('wireviz', 'console_scripts', 'wireviz')()
  File "/usr/lib/python3/dist-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/usr/lib/python3/dist-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/lib/python3/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/mm/Dokumentumok/BlackEye/loopback/drawings/WireViz/src/wireviz/wv_cli.py", line 137, in wireviz
    wv.parse(
  File "/home/mm/Dokumentumok/BlackEye/loopback/drawings/WireViz/src/wireviz/wireviz.py", line 88, in parse
    yaml_data, yaml_file = _get_yaml_data_and_path(inp)
  File "/home/mm/Dokumentumok/BlackEye/loopback/drawings/WireViz/src/wireviz/wireviz.py", line 397, in _get_yaml_data_and_path
    raise e
  File "/home/mm/Dokumentumok/BlackEye/loopback/drawings/WireViz/src/wireviz/wireviz.py", line 389, in _get_yaml_data_and_path
    yaml_path = Path(inp).expanduser().resolve(strict=True)
  File "/usr/lib/python3.8/pathlib.py", line 1181, in resolve
    s = self._flavour.resolve(self, strict=strict)
  File "/usr/lib/python3.8/pathlib.py", line 363, in resolve
    return _resolve(base, str(path)) or sep
  File "/usr/lib/python3.8/pathlib.py", line 347, in _resolve
    target = accessor.readlink(newpath)
  File "/usr/lib/python3.8/pathlib.py", line 452, in readlink
    return os.readlink(path)
OSError: [Errno 36] File name too long: "/home/mm/Dokumentumok/BlackEye/loopback/drawings/WireViz/ \nconnectors:\n  'aand here comes the YAML content

It looks that some paramteres swapped. Is it worth troubleshooting it or is there a better branch to start with the SVG embed?

jakew009 commented 2 years ago

Same issue as @martonmiklos

kvid commented 2 years ago

I've not tested the #189 code myself after it was rebased into the latest chain of PR branches, but the https://github.com/formatc1702/WireViz/pull/189#issuecomment-944955954 explains that there exists an alternative branch backup/bugfix/image-src_before-latest-rebase that might help you.

Either the alternative branch can be used directly, or the two branches can be compared to investigate differences.

amotl commented 2 years ago

Dear Daniel,

it is sweet to see you working on #186 and #251 ff. again. Just let me know when you converged this into a new release, and I will update WireViz-Web correspondingly.

With kind regards, Andreas.

kvid commented 12 months ago

My contributions to help with filalizing #251 is work in slow progress due to an increase in the effort I have to use on other important stuff in my life this autumn. Hence, I propose to consider creating a release 0.4 BEFORE merging in #251 - to let all users try out the new functionality now, including other projects that depend on this projepct. It has already been way too long since our last release. There is no problem also adding an extra release shortly after merging in #251 if needed.

I've already made an effort on this code by merging in a few extra PRs after the huge effort by @formatc1702 this summer, and fixing some minor stuff, so generating examples should work now.

I encourage @amotl and also other motivated users/contributors to try out the current dev branch and look out for minor things that are easy to fix before a release, like e.g. any missing or wrong documentation of new features. I also consider merging #298 into dev before such a release, as it is a low hanging fruit and should be easy to integrate with #251 later.

Edit: Should we also consider to merge in #309 (maybe after some adjustments) to make the release I suggest more up-to-date on the Python versions in use today?

formatc1702 commented 11 months ago

I very much support a release of v0.4 before #251. Including #298 should be no problem. I can take care of publishing onto PyPI once I get the signal.

Adding support for current Python versions (#309), and even dropping support for deprecated ones, should help in keeping the project fresh and not bound to older syntax conventions etc. I have not been actively following the updates but I do not want the project to be bloated by feeling forced to support old versions.

Thank you for pushing forward, but don't let the project get in the way of life either! 💪

kvid commented 9 months ago

IMHO, we need to fix #328 before a new release. Please comment my suggested fix with a new option in that issue.

formatc1702 commented 6 months ago

I have submitted #332 as a response to #328.

I would like to go forward with merging #332 and #298 into dev, publishing v0.4 and afterwards immediately rebasing big-refactor onto dev and fast-fowarding, so finally we have a dev branch with more manageable PRs again.

formatc1702 commented 6 months ago

I have merged #332 and #298, updated the version number and added a v0.4-rc tag.

[Edit 2024-05-05]: Renamed tag to v0.4-rc to distinguish from proper v0.4 once it is merged into master.

However, I consider this an interim version that will not be merged into master or published on PyPI, and the changelog reflects this.

The next immediate step is rebasing #251 onto the latest dev branch, now labeled v0.5-dev to allow differentiating between changes from before (v0.4) and after (v0.5) the refactoring. I would appreciate any help in doing so. Then dev can be moved forward to contain the refactored code to finally get rid of big-refactor even if not all kinks are fully ironed out yet.

kvid commented 6 months ago

I have merged #332 and #298, updated the version number and added a v0.4 tag.

Thank you!

However, I consider this an interim version that will not be merged into master or published on PyPI, and the changelog reflects this.

I don't understand why v0.4 should not be merged into master and released the normal way. My main motivation for suggesting v0.4 before merging #251 was to make it available to users and getting more usage feed-back.

It shouldn't be much work taking this remaining step to a full release, and I see no drawbacks about it, unless you know about any critical bugs or documentation flaws in v0.4.

The next immediate step is rebasing #251 onto the latest dev branch, now labeled v0.5-dev to allow differentiating between changes from before (v0.4) and after (v0.5) the refactoring. I would appreciate any help in doing so. Then dev can be moved forward to contain the refactored code to finally get rid of big-refactor even if not all kinks are fully ironed out yet.

I agree to this step, but AFTER a merge of v0.4 into master.