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

Add optional tweak per node with name placeholder #357

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented May 22, 2024

Implements feature suggested in #349

@kvid
Copy link
Collaborator Author

kvid commented May 22, 2024

See my newly extended #270 (comment) for a use case with example YAML input for this PR.

@kvid kvid linked an issue May 22, 2024 that may be closed by this pull request
@martinrieder
Copy link
Contributor

martinrieder commented May 23, 2024

#349 (comment)

Great to see this implemented! Now I understand how you intended to do that search & replace, though I still do not understand what the placeholder would be needed for. I think that it might make sense to have a prefix to an auto-generated designator though... What is the advantage of having a user-defined ID here?

I also realized that the templates and examples do not contain any tweaks. The automated checks would only confirm that this PR does not break any of them.

@kvid kvid force-pushed the issue349tweak branch from 3564614 to b82c7fe Compare May 29, 2024 22:19
@martinrieder
Copy link
Contributor

martinrieder commented May 30, 2024

I wrote:

I also realized that the templates and examples do not contain any tweaks. The automated checks would only confirm that this PR does not break any of them.

@kvid :
Would it make sense to include some additional examples for these tweaks with the PR, i.e. a simplified version of #325?
In case that you want to leave them undocumented (refer to Graphviz docs), should there be an additional testing folder for the GH actions to capture any issues?

Taking the latter thought a bit further, I come to think of it as regression testing for solved issues... Quite a bit of work to implement though.

@kvid
Copy link
Collaborator Author

kvid commented May 30, 2024

@martinrieder wrote:

[...]

I also realized that the templates and examples do not contain any tweaks. The automated checks would only confirm that this PR does not break any of them.

True

@kvid : Would it make sense to include some additional examples for these tweaks with the PR, i.e. a simplified version of #325?

It would perhaps be nice with an example, but bear in mind that tweaks are mainly needed for workarounds when the ordinary functionality is not sufficient or contains errors, or to test possible new features. Hopefully, such needs will be resolved by extending the ordinary functionality in the future.

In case that you want to leave them undocumented (refer to Graphviz docs),

Creating tweaks does require knowledge of the Graphviz syntax, and that is a too large subject to cover in the WireViz documentation, so referring to Graphviz docs is needed.

should there be an additional testing folder for the GH actions to capture any issues?

#251 will add test folders that might be what you seek.

Taking the latter thought a bit further, I come to think of it as regression testing for solved issues... Quite a bit of work to implement though.

See #63

@martinrieder
Copy link
Contributor

In case that you want to leave them undocumented (refer to Graphviz docs),

Creating tweaks does require knowledge of the Graphviz syntax, and that is a too large subject to cover in the WireViz documentation, so referring to Graphviz docs is needed.

I would put some notes into syntax.md and amend those to my PR #386 then. We have seen a few requests about clustering. That is something that I would still like to add, besides the hint about the image attributes.

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.

[feature] Add tweak option per node
2 participants