Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

[E-export < T338] Implement export_csv_graph - #325

Merged
antoniofilipovic merged 11 commits into
E-add-export-functionsfrom
T338-MAGE-csv_graph
Sep 7, 2023
Merged

[E-export < T338] Implement export_csv_graph#325
antoniofilipovic merged 11 commits into
E-add-export-functionsfrom
T338-MAGE-csv_graph

Conversation

@mpintaric55334

Copy link
Copy Markdown
Contributor

Description

Please briefly explain the changes you made here.

Pull request type

  • Bugfix
  • Algorithm/Module
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related issues

Delete if this PR doesn't resolve any issues. Link the issue if it does.

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

######################################

@mpintaric55334 mpintaric55334 added the status: ready PR is ready for review label Aug 23, 2023
@mpintaric55334 mpintaric55334 self-assigned this Aug 23, 2023
@antoniofilipovic
antoniofilipovic self-requested a review August 28, 2023 09:42

@antoniofilipovic antoniofilipovic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, I suggested few changes what to do

Comment thread e2e/export_util_test/test_export_csv_graph/input.cyp Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
@antoniofilipovic antoniofilipovic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Aug 28, 2023
@mpintaric55334 mpintaric55334 added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Aug 29, 2023

@antepusic antepusic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, left a few comments. Two of them relate to several files so I’m posting them here:

  • Line breaks: csv.writer chooses the system-appropriate line break, but it’s safer to use split(data, "\n") in the tests (some systems use just "\n", and trim will remove the "\r" anyway)
  • I think the tests would be easier to follow if you changed the last line of the test query to RETURN row[1] AS labels, row[7] AS type;

Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread e2e/export_util_test/test_export_csv_graph1/test.yml Outdated
Comment thread e2e/export_util_test/test_export_csv_graph2/test.yml Outdated
Comment thread python/export_util.py
@antepusic antepusic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Aug 30, 2023
@mpintaric55334 mpintaric55334 added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Sep 4, 2023
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated

@antoniofilipovic antoniofilipovic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Last few changes

@antoniofilipovic antoniofilipovic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Sep 5, 2023
@mpintaric55334 mpintaric55334 added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Sep 6, 2023

@antoniofilipovic antoniofilipovic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me

@antepusic antepusic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved 🚀
Before merging, just make sure to rename separateHeader and address this previous comment of mine.

Comment thread python/export_util.py
@antoniofilipovic
antoniofilipovic merged commit 9e7dadc into E-add-export-functions Sep 7, 2023
@antoniofilipovic
antoniofilipovic deleted the T338-MAGE-csv_graph branch September 7, 2023 07:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

status: ready PR is ready for review type: module

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants