search

oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.8k stars 4.01k forks source link

[Incomplete] Adding/Updating 'Args', 'Raises', and 'Return' in docstrings #11776

Open hardikkat24 opened 3 years ago

hardikkat24 commented 3 years ago

Aim/Problem: Improve the docstrings in all the python files in the codebase. This will also fulfill the prerequisite for the lint check Check to enforce use of Args, Returns and Raises in the docstring. to be introduced for #8423.

Suggested fix: Do a grep or a search (CTRL+F) for def keyword in the file to get all the possible places (i.e functions and methods) that we would need to fix. Verify that the correct pattern for docstring (which includes Args, Return, and Raises if applicable as shown below) exists for all those functions/methods, and if it is not already present, then edit their doc string. An example of a good docstring is shown below:

def someFunction(param1, param2):
    """This is a sample of a correct docstring.
    Args:
         param1: <type>. <details>.
         param2: <type>. <details>.

    Returns:
        <type>. <details>.

    Raises:
        <error_type>. <details>.
    """
    # <rest of the function >

where, Args denotes all parameters with its type and details, Raises denotes all the types of errors/exceptions that could be raised, and Returns denotes the details about what a function returns.

Important: Docstrings are not required for test and setUp functions in the test files(files that end with _test.py).

To be assigned one of the below tasks, leave a comment at-mentioning @SD-13 and specify which one you want to take up.

The list of files that need improvement (Please create a PR that fixes all the files assigned under one checkbox):

Siddhant-K-code commented 3 years ago

Can I try to fix this?

sajalasati commented 3 years ago

Can I try to fix this?

@Siddhant-K-code Sorry for delayed response. Actually no one was assigned to this issue earlier. If you are still interested to work on it, can you pick one task (checkbox) and I will assign it to you. (I have also updated the issue description, so kindly go through that once more to understand the task). Thanks!

github-actions[bot] commented 3 years ago

Hi @sajalasati, thanks for proposing this as a good first issue. I am removing the label for now and looping in @vojtechjelinek to approve the label. It will be added back if approved. Thanks!

sajalasati commented 3 years ago

Assigned myself to keep track of the progress made on this issue.

aasiffaizal commented 3 years ago

@sajalasati If the first Opia checkbox isn't claimed by anyone, I would like to work on it.

hardikkat24 commented 3 years ago

@aasiffaizal I have assigned you.

KartikKapil commented 3 years ago

@sajalasati Could i work on the second check box (i.e oppia.core)

sajalasati commented 3 years ago

@sajalasati Could i work on the second check box (i.e oppia.core)

@KartikKapil I've assigned you :)

Siddhant-K-code commented 3 years ago

Yes, I would like to work on this, I can work on whole files ( except 1st and 2nd {already Assigned } ). Please Assign me, @sajalasati

sajalasati commented 3 years ago

Yes, I would like to work on this, I can work on whole files ( except 1st and 2nd {already Assigned } ). Please Assign me, @sajalasati

@Siddhant-K-code There are approx 280+ files remaining, and if you cover it in a single PR it will become very large and will take long time to review. I would advice taking individual checkboxes for now, or I am even fine if you want to pick one entire folder at a time like oppia.core.controllers (which has 5 parts/checkboxes) or oppia.core.domain (10 parts). Let me know, and I will assign you. (I can assign you more in future as you start making PRs)

Siddhant-K-code commented 3 years ago

Ok, I will start with 3rd. !

sajalasati commented 3 years ago

Ok, I will start with 3rd. !

@Siddhant-K-code Assigned you to 3rd checkbox - oppia.core.controllers. Do Let us know if you have any questions!

KartikKapil commented 3 years ago

@sajalasati i have a query, while making the doc string if a fuction returns a list which contains a model should i write it returns a list or should i also write what the returning model does ?

sajalasati commented 3 years ago

@sajalasati i have a query, while making the doc string if a fuction returns a list which contains a model should i write it returns a list or should i also write what the returning model does ?

@KartikKapil You can look at other examples in the file to get an idea - generally we write it in the docstring as Returns: list(<data_type>) (link) where data type could also be the name of a model.

KartikKapil commented 3 years ago

@sajalasati Thank you

KartikKapil commented 3 years ago

@sajalasati could i ping you gitter? Regarding this issue

sajalasati commented 3 years ago

@sajalasati could i ping you gitter? Regarding this issue

@KartikKapil I am not sure if I can be available much on gitter. You can use this thread to ask all your doubts without any worry, and we will try to clear them or guide you to proper resources/wiki pages (it will only help future contributors working on this issue to get things cleared early on). If you have setup related questions you can use oppia-chat on gitter. We encourage usage of public chats more than DMs :)

KartikKapil commented 3 years ago

@sajalasati i have edited all the three files , i have a few worries in oppia.core.platform_feature_list_test.py file there are functions that have no arguments or return value , should i leave those function as it is ?

sajalasati commented 3 years ago

@sajalasati i have edited all the three files , i have a few worries in oppia.core.platform_feature_list_test.py file there are functions that have no arguments or return value , should i leave those function as it is ?

@KartikKapil Thanks for checking!

You should include Args in the docstring if the function has any parameters, include Returns in the docstring if it has a return statement, else you don't need to make any changes. You can also look at the description comment for the lint check for which we are fixing all these files if you want.

Also just an advice - if you have doubts for specific files/functions, you should either provide a link to (a few of) those while asking question so that we can understand what the case is. It's also fine to create a PR with those modifications if you feel confident about it (reviewer will correct you then), or leave a comment in PR for specific places you are doubtful about.

KartikKapil commented 3 years ago

@sajalasati Sure i would do that , thank you so much for helping me here .

Siddhant-K-code commented 3 years ago

@sajalasati Please review #12250

KartikKapil commented 3 years ago

@sajalasati i could not pass some of these linter checks because they ask a new line above the Args arugument and when i give that it gives a critical error that first line cannot be empty how do i fix that kindly help ? thanks in advance , here is the screenshot of my terminal . errors

hardikkat24 commented 3 years ago

@KartikKapil This might happen if you don't start with a description and straightaway start with Args. Try adding a description then blank line, then Args and so on.

nisargsc commented 3 years ago

@sajalasati @arpit1912 I would like to work on this. Can you please assign the 4th checkbox to me (oppia.core.controllers: PART 2) Thanks!!

KartikKapil commented 3 years ago

@sajalasati @hardikkat24 i have managed to debug most of the error could you help me with this last one i could not really understand it. Thanks again errors

hardikkat24 commented 3 years ago

@KartikKapil You might have added get-pip.py file to get pip installed on your system. Please delete that file or move it to some other directory. This should solve the problem.

KartikKapil commented 3 years ago

@hardikkat24 @sajalasati kindly reviw #12293

hardikkat24 commented 3 years ago

@Nisarg-Chaudhari I have assigned you.

lkbhitesh07 commented 3 years ago

@hardikkat24, Will you please assign me oppia.core.controllers : PART 4? Thanks Also, I have one question, l was looking at the skill_editor.py file and now I want to know the type of Args.

class SkillDataHandler(base.BaseHandler):
    """A handler for accessing skills data."""

    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

    @acl_decorators.open_access
    def get(self, comma_separated_skill_ids):
        """Populates the data on skill pages of the skill ids."""

        skill_ids = comma_separated_skill_ids.split(',')

        try:
            for skill_id in skill_ids:
                skill_domain.Skill.require_valid_skill_id(skill_id)
        except utils.ValidationError:
            raise self.PageNotFoundException('Invalid skill id.')
        try:
            skills = skill_fetchers.get_multi_skills(skill_ids)
        except Exception as e:
            raise self.PageNotFoundException(e)

        skill_dicts = [skill.to_dict() for skill in skills]
        self.values.update({
            'skills': skill_dicts
        })

        self.render_json(self.values)

Now for this function, if I want to know that where SkillDataHandler.get() is being used in the frontend so that I can check the type of the Args by console.log(). I'm confused on how to figure out because git grep is not much of a help here.

Also one more question, if I want to check Args type for a test file, then how am I suppose to do that, I mean how to check which specific test will trigger that file? Thanks

sajalasati commented 3 years ago

Hello @hardikkat24! A few tasks for you related to this issue:

I think we had a discussion earlier whether to enforce this lint check on test files or not, but it remained incomplete then. We need to be sure about that so ensure all files fixed in a PR and properly fixed at once.

Would you be able to create a small PR so that we can once verify the approach (and code) you used for finding the above list of files? The lint error log of that PR can serve as a checklist to ensure we are not missing any file for correction.

I think the best way ahead would be to create a small design doc for the change we want:

After that:

@hardikkat24 Would you be interested to complete this? If yes, then we should start working on it soon. Let me know where you need help. Thanks!

lkbhitesh07 commented 3 years ago

Thanks, @sajalasati.

sajalasati commented 3 years ago

@aasiffaizal @KartikKapil @Siddhant-K-code Since you have made your PRs, I will review them soon, and let you know if additional changes are required.

@Nisarg-Chaudhari @lkbhitesh07 Since you have not made a PR yet, I would suggest you to work on a different issue, as we realised about some of the requirements later, and it would hard (and time taking) to do a proper review and get it merged. Un-assigning you both for now. Extremely sorry for the inconvenience.

lkbhitesh07 commented 3 years ago

No problem at all @sajalasati, thanks for the updates.

nisargsc commented 3 years ago

@sajalasati No problem, Thanks for updating.

hardikkat24 commented 3 years ago

Hello @hardikkat24! A few tasks for you related to this issue:

I think we had a discussion earlier whether to enforce this lint check on test files or not, but it remained incomplete then. We need to be sure about that so ensure all files fixed in a PR and properly fixed at once.

Would you be able to create a small PR so that we can once verify the approach (and code) you used for finding the above list of files? The lint error log of that PR can serve as a checklist to ensure we are not missing any file for correction.

I think the best way ahead would be to create a small design doc for the change we want:

* Propose your solution for enforcing the lint check.

* Show what kinds of lint errors you get in the codebase upon introducing it and get reviews from core maintainers on whether we should include lint check for test_files or not based on that.

* Refine your approach if needed, get approval, create a PR and get the list of files to be fixed.

After that:

* Edit this issue (so that a contributor can make changes to linter for enabling those options, find and fix all lint errors in their assigned files, verify the file has been fixed, then undo the changes in linter).

* Start assigning contributors to tasks again! Because only then we would be sure of the required fix that will enable us to introduce the lint check. (**Don't assign more contributors for now**)

@hardikkat24 Would you be interested to complete this? If yes, then we should start working on it soon. Let me know where you need help. Thanks!

@sajalasati I would love to do this. But I am currently working on my GSoC proposal and have 2 issues assigned. So sorry for this.

NamanJain1902 commented 3 years ago

@sajalasati Could I work on controllers PART-2?

sajalasati commented 3 years ago

@sajalasati Could I work on controllers PART-2?

@nj1902 Sorry the issue description is a bit incomplete currently and I am not assigning anyone until it is complete. Could you please pick some other issue meanwhile? Thanks!

NamanJain1902 commented 3 years ago

@sajalasati No problem, Thanks for updating.

aasiffaizal commented 3 years ago

@sajalasati @hardikkat24 My changes are merged #12287, can you update the description?

Siddhant-K-code commented 3 years ago

Also, please review #12250

sajalasati commented 3 years ago

@sajalasati @hardikkat24 My changes are merged #12287, can you update the description?

Done!

sajalasati commented 3 years ago

Also, please review #12250

@Siddhant-K-code Left a comment to resolve your query for fixing the lint errors. You need to make changes to fix those first before a review.

DubeySandeep commented 3 years ago

@sajalasati @hardikkat24, I don't think we need docstring for the test and setUp function in the _test.py file, can you please confirm and mention this very clearly in the issue description?

hardikkat24 commented 3 years ago

@DubeySandeep Done!

seonWKim commented 3 years ago

Can I try oppia.scripts : PART 1 ?

hardikkat24 commented 3 years ago

@seonwoo960000 Sorry, the issue description is not complete yet. We cannot assign you. Could you pick any other issue? Thanks!

seonWKim commented 3 years ago

@hardikkat24 Sure. Can I try [oppia.core.controllers : PART 2] then ?

sajalasati commented 3 years ago

@hardikkat24 Sure. Can I try [oppia.core.controllers : PART 2] then ?

For all the tasks we are working on completing the requirements for the issue, so that it's straightforward for contributors to solve it. Maybe you can take a look at other issues tagged with good-first-issue label.

seonWKim commented 3 years ago

@hardikkat24 Sure. Can I try [oppia.core.controllers : PART 2] then ?

For all the tasks we are working on completing the requirements for the issue, so that it's straightforward for contributors to solve it. Maybe you can take a look at other issues tagged with good-first-issue label.

Ok~ thanks !

oppiabot[bot] commented 3 years ago

Hi @oppia/core-maintainers, this issue is not assigned to any project. Can you please update the same? Thanks!