WireViz
WireViz copied to clipboard
[feature] Define a new dataclass with color and font attributes
- This PR aims to solve #225 by collecting color and font attributes into a dataclass for visual appearance.
- This will break a few parts of the syntax in v0.3 to simplify both syntax and the data structure.
- e.g.
options.bgcolor_title
->options.title.bgcolor
- e.g.
Thanks for tackling the issue with more customizable looks! WireViz dark mode FTW!
Please bear in mind that -as you may or may not have seen-, I am working on a major refactoring on the entire WireViz codebase in #251, so I would prefer to implement this feature on top of the freshly refactored code.
The best approach to get this merged will likely look like this:
- You can keep working on this PR based on the stable
dev
branch until you are satisfied and are ready to submit - However, please don't expect this new PR to be merged before
dev
has caught up with the refactoring- (I would like to avoid merging this in
dev
before, and then having to rebase the entire refactoring on top)
- (I would like to avoid merging this in
- Once the refactoring is complete, we adapt the PR to work with, and rebase on top of, the refactored code
- It would be risky to rebase already, because some upcoming changes might force you to needlessly refactor multiple times.
The refactoring should change little in the logic required for this PR and in the result for the user, but a lot in the actual implementation... but reworking it should be relatively straightforward.
Thanks for tackling the issue with more customizable looks! WireViz dark mode FTW!
I started this branch in April, and tried a few approaches, but decided to wait until both #214 and #219 was merged, because I needed both of these to complete. I picked up the thread again 3 weeks ago, but got too busy with other things before I managed to create a PR. I observed a lot of activity these last weeks, including a release, but I had very limited time, and have not yet looked into all the new stuff.
I decided to create this as a draft PR to let you know what I have in mind. One of the reasons it's not yet completed, is that I suggest to put in inheritance in the dataclasses, but that makes certain things more complicated:
- It might be an advantage to see how this fits together with a refactoring of all dataclasses to inherit common superclasses instead of duplicating all the common attributes, like I suggested in issue https://github.com/formatc1702/WireViz/issues/56#issuecomment-714798552. I don't know yet if such a thing might also be part of your #251.
- There are a few attributes without default value in some of the dataclasses, and that creates a problem when such a dataclass should inherit from a superclass that contains any attribute with a default value. The simplest solution is probably to assign a None default value and throw an exception if such a None value actually appear in
__post_init__()
, but maybe you suggest a different work-around.
Please bear in mind that -as you may or may not have seen-, I am working on a major refactoring on the entire WireViz codebase in #251, so I would prefer to implement this feature on top of the freshly refactored code.
The best approach to get this merged will likely look like this:
- You can keep working on this PR based on the stable
dev
branch until you are satisfied and are ready to submit- However, please don't expect this new PR to be merged before
dev
has caught up with the refactoring
That's also what I have thought, but in case your #251 also involves an inheritance structure of dataclasses, there might exist solutions that will benefit both these PRs.
(I would like to avoid merging this in
dev
before, and then having to rebase the entire refactoring on top)Once the refactoring is complete, we adapt the PR to work with, and rebase on top of, the refactored code
- It would be risky to rebase already, because some upcoming changes might force you to needlessly refactor multiple times.
That seems reasonable. That's also one of the reasons for me to make this a draft PR at this moment.
The refactoring should change little in the logic required for this PR and in the result for the user, but a lot in the actual implementation... but reworking it should be relatively straightforward.
I look forward to have a closer look into the #251 PR when I have the time to spend.
I look forward to have a closer look into the #251 PR when I have the time to spend.
You're welcome to have a look anytime, but expect the unexpected (missing features, unexpected output, etc.) while the PR is in draft status ;)