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

[refactor] Use the csv library to generate properly escaped TSV files #116

Closed
wants to merge 4 commits into from

Conversation

aakatz3
Copy link
Contributor

@aakatz3 aakatz3 commented Jul 21, 2020

This closes #98 and allows escaping tabs in TSV files. It also uses library functions to generate the CSV and TSV files, instead of a quick custom solution, for robustness.

@formatc1702
Copy link
Collaborator

Thanks, will check it out later. Looks like a good addition.
Did you confirm the TSV output remains the same as it previously was?

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jul 21, 2020

TSV output matches exactly, other than fixing a bug where if a tab was present inside of any of the data fields, it was previously not escaped. Now, the field is quoted. If you would like, I can restore the original behavior.

@formatc1702
Copy link
Collaborator

Great that it matches exactly! And even better if it escapes existing tabs, that's much better. Let me test it a bit and I'll merge.

Comment on lines 29 to 40
def generate_bom_outputs(base_filename, bomdata, *argv):
expanded_csv_names = len(_csv_formats.intersection(set(argv))) > 1
expanded_tsv_names = len(_tsv_formats.intersection(set(argv))) > 1
for fmt in argv:
if fmt in _csv_formats:
file = csv.writer(open_file_write(base_filename + ("_" + fmt.__name__ if expanded_csv_names else "") + _csv_ext, fmt.lineterminator), fmt)

elif fmt in _tsv_formats:
file = csv.writer(open_file_write(base_filename + ("_"+fmt.__name__ if expanded_tsv_names else "") + _tsv_ext, fmt.lineterminator), fmt)
else:
raise KeyError("Unknown BOM Format Specified")
file.writerows(wv_helper.flatten2d(bomdata))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *argv parsing (lines 29 to 31) are not very pythonic, I'd request you change it.
The len(intersection(set())) > 1 is rather cryptic... and the use case of calling this function requesting multiple CSV and TSV formats at once is pretty much an edge case IMHO... especially since currently there are set defaults, inaccessible to the user.

My proposal: Call the function with a list as the third argument:

def generate_bom_outputs(base_filename, bomdata, formats):
    if not isinstance(formats, List):
        formats = [formats]

and then iterate over the list.
It could then be called using

bom_helper.generate_bom_outputs(filename,bom_list,[bom_helper.WIREVIZ_TSV, bom_helper.EXCEL_CSV])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work well! I will also check briefly to see if the Excel TSV format is essentially compatible with the one wireviz natively generates (and probably have the OS choose if new lines are unix or windows style), and then the csv formats can be directly passed through.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, excuse any python style issues, I will try to fix them whenever pointed out. I'm a Java / C / MATLAB developer, primarily, who has wanted to learn python for far too long!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a Java / C / MATLAB developer, primarily, who has wanted to learn python for far too long!

You're not alone, I've done mostly embedded programming in C++ / Arduino before.
I'm loving Python's readability and trying to keep WireViz as pythonic and understandable as possible :)

raise KeyError("Unknown BOM Format Specified")
file.writerows(wv_helper.flatten2d(bomdata))

# TODO: Possibly refactor other BOM output operations, such as HTML, into here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in a separate PR :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion in #123

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, leaving comment for future reference, unless requested otherwise.

with open_file_write(f'{filename}.bom.tsv') as file:
file.write(tuplelist2tsv(bom_list))
# todo: support user choices of BOM format (probably also graphviz outputs, html outputs)
bom_helper.generate_bom_outputs(filename,bom_list,bom_helper.WIREVIZ_TSV, bom_helper.EXCEL_CSV)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in bom_helper.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, leaving comment for future reference, unless requested otherwise.

src/wireviz/wv_helper.py Outdated Show resolved Hide resolved
@formatc1702 formatc1702 mentioned this pull request Jul 24, 2020
@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 24, 2020

Please see my proposal in #123.

I encourage you to submit this pull request to @slightlynybbled's fork, as he's willing to integrate the new CSV/TSV output into a refactored version that implements a clean CLI and better separation of wiring harness logic from file output.
This should also address the remaining TODOs in your code.

Please comment there and close this PR if you are in.

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jul 24, 2020

Will do, resolving your issues first then opening new pull request.

@aakatz3 aakatz3 marked this pull request as draft July 24, 2020 19:34
@aakatz3 aakatz3 closed this Jul 24, 2020
@aakatz3 aakatz3 reopened this Jul 24, 2020
@formatc1702
Copy link
Collaborator

Please make sure to submit the new PR to slightlynybbled's fork, not here. That way he can take care of integrating it, and then submit it together with his CLI improvements onto the original WireViz repo.

@wireviz wireviz locked as resolved and limited conversation to collaborators Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants