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 version number to output files with meta info #177

Merged
merged 3 commits into from
Oct 16, 2020

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Oct 14, 2020

This is a follow-up to the version number part of #173 to also tag the .gv and .html output files with version number.
The last few HTML validation errors remaining from #97 was also removed in the process.

@kvid kvid marked this pull request as draft October 14, 2020 21:04
@kvid kvid force-pushed the improve-output-metainfo branch from 7d83db5 to 4f9de29 Compare October 14, 2020 21:07
@kvid kvid marked this pull request as ready for review October 14, 2020 21:08
@kvid kvid changed the title Add version number to the meta generated tag Add version number to output files with meta info Oct 14, 2020
src/wireviz/Harness.py Outdated Show resolved Hide resolved
@formatc1702 formatc1702 added this to the v0.2 milestone Oct 15, 2020
@formatc1702
Copy link
Collaborator

This is a good idea and I'm willing to include it in v0.2!

@kvid kvid marked this pull request as draft October 15, 2020 17:05
@kvid kvid force-pushed the improve-output-metainfo branch from 3890a3a to ddbda98 Compare October 15, 2020 19:08
@kvid kvid marked this pull request as ready for review October 15, 2020 19:09
@kvid
Copy link
Collaborator Author

kvid commented Oct 15, 2020

I'm sorry that the two last commits are slightly off topic, but the reasoning is:

  • It didn't feel right to define the application name and URL once again when writing the meta tag (ref DRY), and with your Add version number to output files with meta info #177 (comment) in mind, I searched for the application name in the code and found it written a lot of places - written 3 different ways: WireViz, Wireviz, and wireviz. Therefore, I moved the definition to a common file and imported these constants where needed.
  • When validating the HTML meta tag, I first added a title tag to remove one of the errors, and finally I found a minor change of the HTML BOM that removed all the remaining errors. Do you want me to move this last commit to a separate PR, or should I just expand the scope slightly in the PR description?

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 15, 2020

I'm sorry that the two last commits are slightly off topic, but the reasoning is:

  • It didn't feel right to define the application name and URL once again when writing the meta tag (ref DRY), and with your #177 (comment) in mind, I searched for the application name in the code and found it written a lot of places - written 3 different ways: WireViz, Wireviz, and wireviz. Therefore, I moved the definition to a common file and imported these constants where needed.

I appreciate the attention to detail, and I agree having a standardized way to write the app's name (WireViz) is good.
However, defining this as a constant and then referencing it a couple of times, and especially piecing together the URL from it (URL = 'https://github.com/formatc1702/' + APP_NAME), is overdoing it and does not contribute to simplifying the code...
TBH I'd rather just correct the instances where the name/URL occurs, and make sure future PRs stick to the convention... perhaps adding a note to the contribution guidelines.
If the name/URL should ever change, a project-wide search&replace will be easy enough, I don't mind having it hardcoded in 2-3 places.

See my comments in the code. Maybe this feature will increase in value in the future :)

  • When validating the HTML meta tag, I first added a title tag to remove one of the errors, and finally I found a minor change of the HTML BOM that removed all the remaining errors. Do you want me to move this last commit to a separate PR, or should I just expand the scope slightly in the PR description?

Hehe, I doubt this would have bothered anyone, and I doubt that the ALIGN attribute is ever going to truly stop working, but I am OK with fixing the validation error in this way. Up to you to create a new PR or not... I am more concerned with the commit history (which would end up looking pretty much the same either way). If you want to be extra clean about it, I guess a PR is the way to go.

kvid added 3 commits October 16, 2020 20:45
Tag the .gv and .html output files with generator and version number.
The application name and URL was defined several places in the code,
and the name was not written exactly the same everywhere.

By using the same constants everywhere, consistency is obtained.
The https://validator.w3.org/ reported Errors:
The align attribute on the th/td element is obsolete. Use CSS instead.

By replacing align="X" attributes with text-align:X; CSS equivalent,
the validator now completes without any errors or warnings.

This solves the remaining issues from wireviz#97.
@kvid kvid force-pushed the improve-output-metainfo branch from ddbda98 to f6d0f23 Compare October 16, 2020 18:47
@formatc1702
Copy link
Collaborator

Looks good, couldn't find any remaining references that would need fixing.

Much appreciated!

@formatc1702 formatc1702 merged commit fb17eae into wireviz:dev Oct 16, 2020
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.

2 participants