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

added unit tests for check_file_exists() and create_link_reference() #37

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

posiaden123
Copy link

Hey @srz2 ! I've worked with unit tests very briefly, but haven't had much experience with them. To the best of my knowledge, I've added the first two tests requested in #32 , but please let me know if I've made a mistake!

@posiaden123
Copy link
Author

holy **** that should be it, im an idiot sometimes

@srz2
Copy link
Owner

srz2 commented Feb 7, 2021

@posiaden123 thank you for your interest and work you've done. I'd love to give you some guidance before I'd merge your changes in.

First, please remove the .idea and its files you've added. Those are specific to InteliJ. You can add the directory to the .gitignore if you want too

Second, the ResponseFormatter requires the repo_url, but you hardcoded it. I would prefer if it was ingested from the INI file. You dont even have to change anything, most likely, you could likely just ingest the example INI since I have it hardcoded there. Its better to do it this way in case the repo ever changes

Lastly, as a general note on test cases. It would be good to test the negative as well. So it would be good to also test inside of your FileExists class a test_does_file_not_exist.

@posiaden123
Copy link
Author

posiaden123 commented Feb 7, 2021

@srz2 how exactly do you want me to ingest from the praw.ini.example, should it be ['DEFAULT']['App Information']['repo_url'] or am i missing something

@srz2
Copy link
Owner

srz2 commented Feb 8, 2021

@posiaden123 App Information is just a comment in the INI file. Take a look at the Configuration.py. The function init_config_with_ini uses the configparser class. At that point, use something like config['repo_url'] to get the repo url

As a general note, you should play around with the config parser in general and be familiar with it. Its a very convenient tool to know.

Also, I see that you added .idea to the gitignore, but please note that you have to actually remove the .idea files you committed. If you look at the "Files Changed" tab, they are still listed

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.

2 participants