Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/test cases #89
Feature/test cases #89
Changes from all commits
2c37e13
a720b26
7d5a673
6ede11e
d09679f
1a0e299
9b9ba9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function just for this test or could have been used in the actual code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the helpers for the test. Not very proud of what I have done but its a make shift hashmap or dictionary to identify collisions in centers and schools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make it private or mark it as the fixture with pytest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project intends to use pytest, but for some reason these tests are written in unittest 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can convert it to pytest later on but pytest should be able to run unittests as well with no issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be great to use pytest as it brings a lot of power to fixtures management and testing.And, it's not about pytest being able to run unit tests; it's more of the above and consistency. Also, the fact that we intend to use pytest, so it would be nice not to have the disparity. But all good, one step at a time ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name should be descriptive enough so that one doesn't need to write any comments, if one needs to write a comment for a function/test, then usually there is a smell; it might not be the case here, but I think the descriptive test would be better off even looking at the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the python auto-doc string so just writing the comment to stick to consistency makes it easier for autoamated documentations later on. I can remove it but basically its checking if the output file exisits or not :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice not to open and close files everywhere in the code; this means that the testing will be pretty cumbersome. Not to mention the repetitions. This also leads to multiple opportunities for executing files all over the code more open doors for executing malicious file. I would like to know the rationale behind it. IO should be done only once as a data input, not wherever we can. Also, since the data size is small, we can load it in memory and pass it along so we don't have to worry about th reader being exhausted.
I think this TSV parser could be reformatted if we really need one; otherwise, just a couple of functions would have done the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed will optimise this code in my Next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem unnecessary functions; it would be good to not add any functions that we don't need. This becomes unmanageable down the line and then we have functions that don't have tests also which is going to be pretty heavy for anyone to manage these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I beg to differ though. These functions are decoupled and maintained inside utils. In this way we have many other objects inherit the same functionality with less repeated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, are we sure that this function is used elsewhere? Otherwise, leaving around unused and untested functions is a relatively bad idea.