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

Fix part of #3905: Add checks for correct docstring style. #4572

Merged
merged 3 commits into from
Jan 20, 2018
Merged

Fix part of #3905: Add checks for correct docstring style. #4572

merged 3 commits into from
Jan 20, 2018

Conversation

vibhor98
Copy link
Contributor

This PR adds checks for correct """ quotes and non-empty first line for docstrings.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
  • The linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR follows the style guide.
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

seanlip
seanlip approved these changes Jan 20, 2018
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@vibhor98
Copy link
Contributor Author

Thanks, 😄 @seanlip

@vibhor98
Copy link
Contributor Author

@DubeySandeep Why these tests are failing? Please help!

@DubeySandeep
Copy link
Member

@vibhor98, some has restarted your tests (I think that must be some flaky error). :)

@apb7
Copy link
Contributor

apb7 commented Jan 20, 2018 •
edited
Loading

Hello @DubeySandeep and @vibhor98! The build fails due to this:

************ Module oppia.scripts.release_info
C: 1, 0: First line empty in module docstring (docstring-first-line-empty)

https://travis-ci.org/oppia/oppia/jobs/331126277#L1023

Please see this once. Thanks!

@apb7
Copy link
Contributor

apb7 commented Jan 20, 2018

@DubeySandeep and @vibhor98: Okay, so the error says:

docstring-first-line-empty (C0199): First line empty in %s docstring
Used when a blank line is found at the beginning of a docstring.

I think deleting this blank line: https://github.com/oppia/oppia/blob/develop/scripts/release_info.py#L16
will fix it.
Thanks!

@vibhor98
Copy link
Contributor Author

vibhor98 commented Jan 20, 2018 via email

@DubeySandeep
Copy link
Member

@apb7, thanks for the check! Also, the error is with docstring-first-line-empty so I think the solution would be to shift line 18 to 17 i.e, no empty line after '''
https://github.com/oppia/oppia/blob/develop/scripts/release_info.py#L15-L24

@codecov-io
Copy link

codecov-io commented Jan 20, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@b47892d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4572   +/-   ##
==========================================
  Coverage           ?   44.76%           
==========================================
  Files              ?      382           
  Lines              ?    23195           
  Branches           ?     3724           
==========================================
  Hits               ?    10383           
  Misses             ?    12812           
  Partials           ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b47892d...ff8f22d. Read the comment docs.

@apb7
Copy link
Contributor

apb7 commented Jan 20, 2018

Yes! @DubeySandeep. You are right!

@seanlip seanlip merged commit 7bd47d7 into oppia:develop Jan 20, 2018
@apb7 apb7 mentioned this pull request Jan 21, 2018
Closed
giritheja added a commit to giritheja/oppia that referenced this pull request Jan 23, 2018
@giritheja
Merge remote-tracking branch 'upstream/develop' into questions_p3
5ffc1e1 
* upstream/develop: (77 commits)
  Add test to HintAndSolutionManagerService to check that hints aren't released after a correct answer is submitted. (oppia#4585)
  Chance cancel property in SpeechSynthesisChunkerService into its own variable. (oppia#4591)
  Enable indent for func-expr (oppia#4588)
  Update text of refresher exp modal. (oppia#4584)
  Update content-type for JSON responses. (oppia#4583)
  Prevent hints from being released after correct answer is submitted. (oppia#4580)
  Change fraction placeholder when no integer part is allowed. (oppia#4581)
  Fix oppia#4567: Changed condition to check range enclosed criteria, edited existing test (oppia#4575)
  Fix part of oppia#3905: Added semi-rule (oppia#4576)
  Adds proper message for validation error in skill ID. (oppia#4577)
  Fix part of oppia#3905: Add checks for correct docstring style. (oppia#4572)
  Fix part of oppia#4374: Add docstrings to core.domain.collection_domain. (oppia#4569)
  Add check to prevent 'SpeechSynthesisUtterance is undefined' error. (oppia#4571)
  Fix of oppia#2533: Modified the stripFormatting filter to preserve newlines in RTE cut-and-paste (oppia#4568)
  Fix part of oppia#3905: Add check for console.log statement (oppia#4564)
  Change Java prerequisite to v8 (oppia#4570)
  Fix oppia#1542: RTE Components correctly displayed in answer group header (oppia#4565)
  Fix part of oppia#4374: Add docstrings to core.domain.param_domain (oppia#4551)
  Fix oppia#4555: Address review comments for oppia#4536. (oppia#4556)
  Update changelog, AUTHORS, CONTRIBUTORS and credits. (oppia#4560)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip approved these changes

@DubeySandeep DubeySandeep Awaiting requested review from DubeySandeep

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vibhor98 @DubeySandeep @apb7 @codecov-io @seanlip