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.

Sign up for GitHub

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

Jump to bottom

docs: added doc strings for TaskDao class and its functions #179

Merged
merged 1 commit into from
Feb 21, 2019
Merged

docs: added doc strings for TaskDao class and its functions #179

merged 1 commit into from
Feb 21, 2019

Conversation

jaerjaerbin
Copy link
Contributor

@jaerjaerbin jaerjaerbin commented Jan 21, 2019 •
edited by isabelcosta
Loading

Description

Added doc strings for TaskDAO class and its functions

Fixes #166

Type of Change:

  • Quality Assurance
  • Outreach
  • Documentation

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

N/A

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have made corresponding changes to the documentation

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@sys-bot
Copy link

sys-bot commented Jan 21, 2019

This PR is not linked to any issue. Please follow the template and link it to an issue.

@sys-bot sys-bot closed this Jan 21, 2019
@isabelcosta isabelcosta reopened this Jan 21, 2019
@sys-bot
Copy link

sys-bot commented Jan 21, 2019

This PR is not linked to any issue. Please follow the template and link it to an issue.

@sys-bot sys-bot closed this Jan 21, 2019
@isabelcosta
Copy link
Member

@jaerjaerbin your PR seems fine, @sys-bot is closing it because the PR template isn't being followed, it can be a "#" missing something you removed that @sys-bot is expecting. I can only fix this when I get home at night, you can also fix it if you'd like

@isabelcosta
Copy link
Member

Just so you know @jaerjaerbin you don't need to actually put the link for the issue on GitHub, if you are in the repository context ;) you can just put #166 and GitHub will make the link by itself

@isabelcosta isabelcosta reopened this Jan 21, 2019
@sys-bot
Copy link

sys-bot commented Jan 21, 2019

Please send PRs only to approved issues.

@sys-bot sys-bot closed this Jan 21, 2019
@isabelcosta isabelcosta reopened this Jan 21, 2019
isabelcosta
isabelcosta requested changes Jan 21, 2019
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Thank you for sending this pull request @jaerjaerbin !
You did a great job with it. I just left some corrections, to keep it consistent with the rest of the doc strings made for the project.

Can you please do the changes I requested and squash your commits in the end?
Feel free to ask any question if you have any doubt!

app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
@jaerjaerbin
Copy link
Contributor Author

jaerjaerbin commented Jan 21, 2019 •
edited
Loading

Yes, I will do those changes. But I'm kind of new in this subject. So, how should I squash my commits in the end of this PR?. Thanks for your help by advance.

@jaerjaerbin
Copy link
Contributor Author

Ok, I hit all the change suggestions commit button.

@isabelcosta
Copy link
Member

@jaerjaerbin Thank you for doing the changes! I'm a bit busy now but I will be able to help you at least until next week. Here's a tutorial I always follow for that, although it is on Slack: https://systers-opensource.slack.com/archives/CAE8QK41L/p1528726928000811

I hope you're there, if not here's the link: http://systers.io/slack-systers-opensource/

@jaerjaerbin
Copy link
Contributor Author

@isabelcosta I was following the steps in turorial you provided me and this is what I got when trying to run this command:

$ git branch -u upstream/develop (develop = branch-the-PR-is-merging-to)

image

I'll be waiting for your help next week.

@isabelcosta
Copy link
Member

@jaerjaerbin do git fetch upstream and then that

@jaerjaerbin
Copy link
Contributor Author

Oops! some workspace config lines in run.py I made was accidentally add. I added manually only the task dao file to commit without realize that -am would add all.

image

Now, Investigating how I can revert changes only on that file.

@jaerjaerbin
Copy link
Contributor Author

@isabelcosta check my changes please. Tell me if anything is wrong.

isabelcosta
isabelcosta requested changes Jan 28, 2019
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

You're almost there, I just have a last change request.

Can you please (1) address my last request change please, (2) then squash commits and (3) make the commit follow our commit style guideline

app/api/dao/task.py Outdated Show resolved Hide resolved
@jaerjaerbin
Copy link
Contributor Author

Ok @isabelcosta Sorry for all my mistakes. Thank you so much for being patient with me. I have learned a lot with making this little contribution to this project. Let me know if anything is wrong.

@isabelcosta isabelcosta added the Category: Documentation/Training Improvements or additions to documentation. label Jan 30, 2019
ramitsawhney27
ramitsawhney27 suggested changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@ramitsawhney27 ramitsawhney27 left a comment

Choose a reason for hiding this comment

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

Very generic return messages in the doc strings, please make it more verbose.

@jaerjaerbin
Copy link
Contributor Author

@ramitsawhney27 review it please!

isabelcosta
isabelcosta previously approved these changes Feb 2, 2019
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Good work @jaerjaerbin 🎉
I'm approving this since I don't have anything else to add here.
But I'm sure @ramitsawhney27 will be able to guide you furtherer if there's something to be changed.

isabelcosta
isabelcosta previously approved these changes Feb 4, 2019
View reviewed changes
ramitsawhney27
ramitsawhney27 suggested changes Feb 5, 2019
View reviewed changes
app/api/dao/task.py Outdated Show resolved Hide resolved
app/api/dao/task.py Outdated Show resolved Hide resolved
@jaerjaerbin
Copy link
Contributor Author

@ramitsawhney27 changes applied. Let me know if anything is wrong.

ramitsawhney27
ramitsawhney27 approved these changes Feb 9, 2019
View reviewed changes
Copy link
Contributor

@ramitsawhney27 ramitsawhney27 left a comment

Choose a reason for hiding this comment

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

Great work!

isabelcosta
isabelcosta approved these changes Feb 10, 2019
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

It looks sooo good and descriptive 🎉
Good work @jaerjaerbin and thank you @ramitsawhney27 for your thoughtful review!

@isabelcosta isabelcosta merged commit 80bacb8 into anitab-org:develop Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isabelcosta isabelcosta isabelcosta approved these changes

@ramitsawhney27 ramitsawhney27 ramitsawhney27 approved these changes

Assignees
No one assigned
Labels
Category: Documentation/Training Improvements or additions to documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create doc strings for TaskDao class and its functions
4 participants
@jaerjaerbin @sys-bot @isabelcosta @ramitsawhney27