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

Implement Class Docstrings as per Issue #5 #67 #68

Merged
merged 24 commits into from
Apr 26, 2021

Conversation

calkerns
Copy link

Reference Issue: #5 Classes Docstrings

This was a Joint effort between @AshirGuptash and I.

Key Changes

Add classes.py file
Add preliminary script-based unit test suite for classes
Add --test flag in DYC for automated testing purposes
Apply Black formatter on all files to ensure consistency
The main body of this is in regards to creating a ClassBuilder object to handle adding docstrings to the start of classes. Currently, it only supports adding details on inheritance to the docstring, but can be further be modified to handle other variables such as __init__ parameters or class attributes.

@calkerns calkerns marked this pull request as ready for review April 24, 2021 03:43
@AshirGuptash AshirGuptash mentioned this pull request Apr 24, 2021
@Zarad1993
Copy link
Owner

Thank you @calkerns for submitting a pull request!

Copy link
Owner

@Zarad1993 Zarad1993 left a comment

Choose a reason for hiding this comment

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

Could you check the README to be documenting the latest changes you are adding?

Thank you so much, again!

@calkerns
Copy link
Author

Hi @Zarad1993, you're welcome; thank you for your response! Yes, you are so right; documentation is needed! My colleague and I will get to work on that tomorrow.

@AshirGuptash
Copy link
Contributor

Hi @Zarad1993 , we updated the README with the documentation for the ClassBuilder options as well as clarified the --test flag (renamed it to --skip-confirm) because all it does is bypass the popup editor in the confirmation phase. This has also been added under the Usage section of the README as it is also useful for the end-user, not just for testing purposes.

Let us know if we should make any other changes!

Copy link
Owner

@Zarad1993 Zarad1993 left a comment

Choose a reason for hiding this comment

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

Why do we need the automated_class_tests directory? 🤔
I might be missing something.
Like tests/automated_class_tests/user_input/class_test_1.in. Not sure what that does or it needs to be in the PR

@Zarad1993
Copy link
Owner

Hey guys 👋 , just want to let you know tests are failing.

@AshirGuptash
Copy link
Contributor

Hi @Zarad1993 , my bad about tests failing, I am trying a few things with regards to pytest and am aware of this issue, sorry about that. In regards to the automated_class_tests directory, our original goal of this was to create an easy alternative to pytest for piping user input into DYC and then automatically testing it against oracle files. Currently I am trying to integrate this into the Pytest framework in order to keep consistency.

@Zarad1993
Copy link
Owner

I love it, man! Let me know when it's ready for review. Keep it up, guys!

If you guys have LinkedIn profiles, I'm happy to connect https://linkedin.com/in/zarad1993 . I'd like to give you guys a shoutout

@AshirGuptash
Copy link
Contributor

AshirGuptash commented Apr 25, 2021

Sounds good, also happy to connect on LinkedIn! Also part of the reason we created the automated_class_tests suite is because many of the methods require files either for reading or writing. The test script we created is useful for testing how changes end up looking after being written to a file which (to our knowledge) is difficult to accomplish via the Pytest framework, This can be seen in methods such as extract_and_set_information which directly opens the filename and thus cannot simply be a hard coded input string.

@AshirGuptash
Copy link
Contributor

Hey @Zarad1993 , we repurposed the automated_class_tests folder by converting the bash-script-based test suite into a Pytest item that helps to test methods that deal with file I/O. However, we have currently commented this portion out due to issues with running on Python2.7, but we think this would be more appropriate to address in the updated test suite as part of issue #2. We also removed the automated_class_tests folder as it is no longer necessary, but have included a subfolder in tests named test_case_files to handle the file I/O from Pytest.

We hope this will suffice!

Copy link
Owner

@Zarad1993 Zarad1993 left a comment

Choose a reason for hiding this comment

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

Nice work, thanks folks!

@Zarad1993 Zarad1993 merged commit 99a6f4a into Zarad1993:master Apr 26, 2021
@calkerns
Copy link
Author

@Zarad1993 I would be happy to connect on LinkedIn as well! Thank you for offering and for working with us on this PR!

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.

3 participants