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

chore(common): expose yields as a property. #79

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

Conversation

OmarAlJarrah
Copy link
Collaborator

@OmarAlJarrah OmarAlJarrah commented Jun 30, 2023

🔗 Related Issues

🎶 Notes

  • I have extended the code by exposing one more property, and not touch the already existing returns or main_returns, so it will not break anything on clients side, clients who are depending on returns to get the same result as yields when the Yields section is the only section present, do not need to do any change, and still can use returns for that purpose.

  • In case more tests for other docstring formats are believed to be required, I will go on and implement them, though I think the current tests are enough.

@rr-
Copy link
Owner

rr- commented Jul 1, 2023

The code looks OK; the chore commit message style is incompatible with what we currently have. The tests should really be separate functions rather than assertions interleaved with actual code (even if this deviates from the current practice); then they could be grouped inside a class for readability. Both of these points aren't as important as the CI failing, but that's due to unrelated problems that I tracked in the issue #80. LMK if you wish to do any follow-up changes, or prefer to merge as-is.

@rr-
Copy link
Owner

rr- commented Jul 8, 2023

@OmarAlJarrah

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