Skip to content

Conversation

@alexpvpmindustry
Copy link
Contributor

@alexpvpmindustry alexpvpmindustry commented Apr 7, 2023

Description

migrated from samples/quotes/History.xlsx to samples/quotes/*.csv
removed dependency of from openpyxl import load_workbook in tests/conftest.py

Fixes #188

Checklist

  • My code follows the existing style, code structure, and naming taxonomy
  • I have commented my code, particularly in hard-to-understand areas
  • I have performed a self-review of my own code and included any verifying manual calculations
  • I have added or updated unit tests that prove my fix is effective or that my feature works, and achieves sufficient code coverage. New and existing unit tests pass locally and in the build (below) with my changes
  • My changes generate no new warnings and running code analysis does not produce any issues
  • I have added or run the performance tests that depict optimal execution times
  • I have made corresponding changes to the documentation

Copy link
Member

@DaveSkender DaveSkender left a comment

Choose a reason for hiding this comment

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

First of all, thank you for contributing! I love what you're doing here, long overdue.

In addition to the specific comments, I have a general thought that duplicating these CSV files from the source seems a bit icky, but I also think referencing another repo is not a great practice. Can you think of a way to simply use the source CSV files rather than putting them here? The only reason I suggest it is that these tests mirror the originating DLL tests and I'd really only want to maintain these CSV files there, ideally, and not have to worry about syncing it here too.

@alexpvpmindustry
Copy link
Contributor Author

First of all, thank you for contributing! I love what you're doing here, long overdue.

no problem 😊, just happen to stumble upon this dinosaur fossil 🤣

In addition to the specific comments, I have a general thought that duplicating these CSV files from the source seems a bit icky, but I also think referencing another repo is not a great practice. Can you think of a way to simply use the source CSV files rather than putting them here? The only reason I suggest it is that these tests mirror the originating DLL tests and I'd really only want to maintain these CSV files there, ideally, and not have to worry about syncing it here too.

i see what you meant. I'm not too sure if there is a method for this that works with pytest. but if the testing data doesn't change too often, I think copying the csv files over should probably be good enough.

@DaveSkender DaveSkender changed the title migrate loading samples from History.xlsx to multiple .csv files use CSV test files Apr 8, 2023
@DaveSkender
Copy link
Member

@LeeDongGeon1996 take a quick peek and merge when ready, LGTM.

@DaveSkender DaveSkender self-requested a review April 8, 2023 09:31
Copy link
Collaborator

@LeeDongGeon1996 LeeDongGeon1996 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution !!
I've just left some comments. Please check when you're available :)

@DaveSkender
Copy link
Member

One or more of these files aren’t importing correctly, not sure which ones. Unit tests are blowing up.

@alexpvpmindustry
Copy link
Contributor Author

just to be sure, its in the import stage and not in the testing stage?
in the testing stage I get 7 failed unit tests, but I have no idea why they fail
image
this is the error log for one of the cases
image

@DaveSkender
Copy link
Member

I think so. My hypothesis is based on all tests passing as of ab5cce9 previously.

@alexpvpmindustry
Copy link
Contributor Author

ok i found the issue, there are several csv that don't follow the current parsing rules. ill fix them

@alexpvpmindustry
Copy link
Contributor Author

ok fixed! lets wait for the tests to complete

@LeeDongGeon1996
Copy link
Collaborator

LGTM, thanks!

@DaveSkender DaveSkender merged commit 50c3622 into facioquo:main Apr 13, 2023
@DaveSkender
Copy link
Member

Thanks for contributing @alexpvpmindustry, much appreciated. ❤️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2023
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.

Migrate test data from .xlsx to .csv

3 participants