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

fix csv format #118

Merged
merged 6 commits into from
Aug 16, 2022
Merged

fix csv format #118

merged 6 commits into from
Aug 16, 2022

Conversation

rm-Umar
Copy link
Contributor

@rm-Umar rm-Umar commented Aug 10, 2022

fixed #85

@rm-Umar rm-Umar force-pushed the umar/fix-csv-format branch from 7ad609e to ce140d8 Compare August 10, 2022 06:54
@rm-Umar
Copy link
Contributor Author

rm-Umar commented Aug 10, 2022

Address is the only field that'll break csv froamt, so we can explicitly quote address field to counter this issue

@rm-Umar rm-Umar requested a review from UmanShahzad August 10, 2022 06:55
Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

There could be others, we need a more generic solution than this 😄

@rm-Umar rm-Umar requested a review from UmanShahzad August 16, 2022 07:55
@rm-Umar rm-Umar marked this pull request as draft August 16, 2022 07:59
@UmanShahzad
Copy link
Contributor

Is this WIP or actually ready @rm-Umar ?

@UmanShahzad
Copy link
Contributor

As I expected the CSV writer adds a newline at the end on each write it does, so we have all kinds of weird scenarios happening:

$ ./build/ipinfo bulk --csv -f asn 1.1.1.1 2.2.2.2
ip,asn_id,asn_name,asn_domain,asn_route,asn_type
1.1.1.1,AS13335,"Cloudflare, Inc.",cloudflare.com,1.1.1.0/24,hosting

2.2.2.2,AS3215,Orange S.A.,orange.com,2.2.0.0/16,isp
$ ./build/ipinfo bulk --csv -f asn,asn 1.1.1.1 2.2.2.2
ip,asn_id,asn_name,asn_domain,asn_route,asn_type,asn_id,asn_name,asn_domain,asn_route,asn_type
1.1.1.1,AS13335,"Cloudflare, Inc.",cloudflare.com,1.1.1.0/24,hosting
,AS13335,"Cloudflare, Inc.",cloudflare.com,1.1.1.0/24,hosting

2.2.2.2,AS3215,Orange S.A.,orange.com,2.2.0.0/16,isp
,AS3215,Orange S.A.,orange.com,2.2.0.0/16,isp

@rm-Umar
Copy link
Contributor Author

rm-Umar commented Aug 16, 2022

Is this WIP or actually ready @rm-Umar ?

It's still in progress I'll mark it ready when it's ready.

@UmanShahzad
Copy link
Contributor

Ah okay. Got pinged for review so wasn't sure 😄

@rm-Umar rm-Umar marked this pull request as ready for review August 16, 2022 10:20
@rm-Umar
Copy link
Contributor Author

rm-Umar commented Aug 16, 2022

Ah okay. Got pinged for review so wasn't sure smile

Now it's ready to review.

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

Great!

@UmanShahzad UmanShahzad merged commit 5cd9dd7 into master Aug 16, 2022
@UmanShahzad UmanShahzad deleted the umar/fix-csv-format branch August 16, 2022 10:38
@UmanShahzad
Copy link
Contributor

Will release it tomorrow or the day after hopefully.

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.

Quote fields when printing csv form for bulk results
2 participants