Full coverage for CODEOWNERS in the codebase. #6616

DubeySandeep · 2019-04-17T00:21:10Z

This PR adds code owners to all possible files in the codebase.
Current status:

Full coverage, all the files which are tracked by git.
Update the lint test for the new behavior.

.github/CODEOWNERS

DubeySandeep · 2019-04-17T00:24:08Z

.github/CODEOWNERS

 /core/storage/question/ @aks681
 /core/templates/dev/head/domain/question/ @aks681
 /core/templates/dev/head/pages/question_editor/ @aks681
 /core/templates/dev/head/pages/questions_list/ @aks681

+# Practice session project.
+/core/templates/dev/head/pages/practice_session/ @aks681


Assuming this to be a part of the new structure project.

Perhaps @vinitamurthi should be co-codeowner? Comes under questions.

Sure, FYI - I am also adding /core/templates/dev/head/pages/question_player/ in #6620

DubeySandeep · 2019-04-17T00:25:05Z

.github/CODEOWNERS

 # Skill project.
+/core/controllers/concept_card_viewer*.py @aks681


@aks681 I'm not sure where to place this (As this is also used in the exploration) so can you confirm that we can place this over here?

Yeah, it's fine to place it here.

DubeySandeep · 2019-04-17T00:26:32Z

.github/CODEOWNERS

 /core/templates/dev/head/domain/topics_and_skills_dashboard/ @aks681
 /core/templates/dev/head/pages/topics_and_skills_dashboard/ @aks681


 # Infrastructure.
+/core/controllers/cron*.py @seanlip


@seanlip, should I create a different section for corn, jobs and task or we can place this over here (inside Infrastructure)? &

I think infrastructure is fine.

codecov-io · 2019-04-17T00:29:31Z

Codecov Report

Merging #6616 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #6616   +/-   ##
========================================
  Coverage    93.95%   93.95%           
========================================
  Files          356      356           
  Lines        52462    52462           
  Branches       893      893           
========================================
  Hits         49290    49290           
  Misses        3172     3172

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 0300b9d...b59ecce. Read the comment docs.

DubeySandeep · 2019-04-17T00:31:57Z

@seanlip & @YashJipkate, is there any specific reason for excluding all the files in the root directory from the codeowner tests? (I'm following CODEOWNERS warning message for assigning codeowners to the remaining files.)

seanlip · 2019-04-17T05:35:49Z

.github/CODEOWNERS

 /scripts/ @apb7


 # Exploration project.
 /core/controllers/editor*.py @DubeySandeep
+/core/controllers/reader*.py @aks681
+/core/controllers/resources*.py @seanlip


@vojtechjelinek can take this one, I think.

Yeah, sure.

seanlip · 2019-04-17T05:36:49Z

@seanlip & @YashJipkate, is there any specific reason for excluding all the files in the root directory from the codeowner tests? (I'm following CODEOWNERS warning message for assigning codeowners to the remaining files.)

This sounds like an error; we should extend the tests to those files too.

apb7 · 2019-04-19T18:57:50Z

Hi @DubeySandeep, please bring your PR branch up-to-date with develop. Thanks!

…deowners

… into full-codeowners

…deowners

DubeySandeep · 2019-04-26T02:06:10Z

.github/CODEOWNERS

+/.gitignore @apb7
+/.isort.cfg @apb7
+/.pylintrc @apb7
+/.stylelintrc @apb7


@apb7, why do we have two .stylelintrc files? is there any specific reason?

@apb7, can you please check this one?

I think we had a discussion on this some time back, @DubeySandeep. We have two stylelintrc files -- one for oppia.css and other for css within html files. The two config files use different pre-processors and therefore, are not the same and one cannot be used for the other. Thanks!

DubeySandeep · 2019-04-26T02:11:01Z

.github/CODEOWNERS

 /core/templates/dev/head/pages/footer_js_libs.html @vojtechjelinek
 /core/templates/dev/head/pages/header_css_libs.html @vojtechjelinek
 /core/templates/dev/head/pages/header_js_libs.html @vojtechjelinek
+/gulpfile.js @vojtechjelinek
+/jinja_utils*.py @vojtechjelinek
+/Makefile @vojtechjelinek


@vojtechjelinek, is there any need of this file? (Introduced at #5659)

No, I was just playing with it and accidentaly commited the file.

I've removed this file!

…proper spaces and newlines.

DubeySandeep · 2019-04-26T02:31:14Z

@YashJipkate, Can you please help to review the test pre_commit_linter test file?

seanlip

Generally speaking this LGTM and I think 8-10 secs is OK. Thanks @DubeySandeep!

seanlip · 2019-04-26T03:21:14Z

scripts/pre_commit_linter.py

+
+    Args:
+        root: str. The path from where the function should start walking.
+        exclude_dirs: list(str). A list of dir path which should to be ignored.


seanlip · 2019-04-26T03:21:24Z

scripts/pre_commit_linter.py

+        exclude_dirs: list(str). A list of dir path which should to be ignored.
+
+    Yields:
+        str. list(str). A list of unignored files.


str or list(str)? Pick one.

list(str). Fixed.

seanlip · 2019-04-26T03:21:45Z

scripts/pre_commit_linter.py

+    Yields:
+        str. list(str). A list of unignored files.
+    """
+    dirs, nondirs = [], []


Does nondirs mean filepaths? If so perhaps that would be a better name.

seanlip · 2019-04-26T03:23:29Z

scripts/pre_commit_linter.py

+    """
+    command = ['git', 'check-ignore', '-q', path_to_check]
+
+    # The "git check-ignore <path>" command use to return 0 when the path is


use to return --> returns

seanlip · 2019-04-26T03:25:19Z

scripts/pre_commit_linter.py

-                            print ('WARNING! %s/%s is not covered under '
-                                   'CODEOWNERS' % (root, file_name))
+                            break
+                    if not match and self.verbose_mode_enabled:


If verbose mode is not enabled, we still need to show the viewer which file failed, right? Shouldn't that be stored in a summary message somewhere?

Ah, I've done my whole testing in --verbose mode, Fixed!

YashJipkate

Thanks, @DubeySandeep just some doubts. PTAL!

.gitignore

scripts/pre_commit_linter.py

DubeySandeep · 2019-04-26T17:00:29Z

@apb7, Can you please review this PR as a code owner?

DubeySandeep · 2019-04-29T03:07:15Z

@apb7 & @YashJipkate PTAL!

oppiabot · 2019-04-29T03:52:20Z

Hi @DubeySandeep. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

YashJipkate · 2019-04-29T13:16:44Z

@DubeySandeep Sorry for the delay; I have addressed the comments. PTAL!
Thanks

YashJipkate

This LGTM!
Thanks @DubeySandeep

DubeySandeep · 2019-05-05T18:50:39Z

@apb7, can you please review this PR?

DubeySandeep · 2019-05-08T21:10:43Z

Hi @apb7, PTAL!

DubeySandeep · 2019-05-11T15:10:04Z

@apb7, can we merge this PR?

apb7 · 2019-05-14T10:56:22Z

scripts/pre_commit_linter.py

+    """
+    command = ['git', 'check-ignore', '-q', path_to_check]
+
+    # The "git check-ignore <path>" command use to returns 0 when the path is


Can you please re-phrase this comment?

I'll surely do it. Also, it would be great if you can provide some suggestion on what I should add or a pattern in which I can re-phrase this comment! :)

Maybe something on these grounds:
The "git check-ignore <path>" command returns 0 when the path is ignored otherwise it returns 1. subprocess.call then returns this returncode.

apb7

LGTM just one comment above. Thanks, @DubeySandeep!

vinitamurthi · 2019-05-14T16:04:29Z

.github/CODEOWNERS

 /core/templates/dev/head/domain/question/ @aks681
+/core/templates/dev/head/pages/practice_session/ @vinitamurthi
 /core/templates/dev/head/pages/question_editor/ @aks681
 /core/templates/dev/head/pages/questions_list/ @aks681



Can we add question_player in this with me as the code owner? It was added in #6620

Thanks for raising this @vinitamurthi, I think within last 28 days we have added many new files so I'll add all those files/folders before merging this PR! (otherwise, the lint test will start failing on develop.)

Currently, I've added a label PR: don't merge - HAS MERGE CONFLICTS to avoid merge.

…deowners

Added codeowners for controllers and other backend files.

376bc93

DubeySandeep requested a review from seanlip as a code owner April 17, 2019 00:21

DubeySandeep commented Apr 17, 2019

View reviewed changes

.github/CODEOWNERS Show resolved Hide resolved

DubeySandeep commented Apr 17, 2019

View reviewed changes

Added codeowners for custom_landing_pages controller.

2f9919a

seanlip reviewed Apr 17, 2019

View reviewed changes

Merge branch 'develop' of https://github.com/oppia/oppia into full-co…

8790440

…deowners

kevinlee12 assigned DubeySandeep Apr 23, 2019

DubeySandeep added 5 commits April 23, 2019 07:45

Added full coverage for the codeowners.

51336c5

Merge branch 'develop' of https://github.com/oppia/oppia into full-co…

2afe464

…deowners

Merge branch 'full-codeowners' of https://github.com/DubeySandeep/oppia…

50e469e

… into full-codeowners

Added full coverage for the codeowners.

13ed285

Update dbranch after makingf delete to rename structure.

8a2d4c5

DubeySandeep changed the title ~~[WIP] Full coverage for CODEOWNERS in the codebase.~~ Full coverage for CODEOWNERS in the codebase. Apr 23, 2019

Codeowners for root directories and associated test cahnges.

d3345a6

DubeySandeep requested a review from apb7 as a code owner April 24, 2019 04:18

DubeySandeep added 2 commits April 26, 2019 07:32

Updated the tests and made changes as asked by the reviewers.

da2fa4a

Merge branch 'develop' of https://github.com/oppia/oppia into full-co…

c1229b7

…deowners

DubeySandeep commented Apr 26, 2019

View reviewed changes

DubeySandeep added 2 commits April 26, 2019 07:42

Added self-review changes, like deleting unwwanted variables, adding …

855d3d3

…proper spaces and newlines.

Added proper docstring.

aad64d2

DubeySandeep requested a review from YashJipkate April 26, 2019 02:31

seanlip approved these changes Apr 26, 2019

View reviewed changes

YashJipkate suggested changes Apr 26, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

scripts/pre_commit_linter.py Show resolved Hide resolved

Added review changes and added comments wherever required.

6ac29a0

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Apr 29, 2019

YashJipkate approved these changes Apr 29, 2019

View reviewed changes

DubeySandeep added 3 commits April 29, 2019 23:54

Updated with upstream/develop.

3f25afc

Changed test file name.

db6da97

Fixed filename issues in CODEOWNERS file.

a5eb2b6

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Apr 29, 2019

DubeySandeep assigned apb7 and unassigned seanlip and YashJipkate May 5, 2019

apb7 reviewed May 14, 2019

View reviewed changes

apb7 approved these changes May 14, 2019

View reviewed changes

vinitamurthi reviewed May 14, 2019

View reviewed changes

DubeySandeep added the PR: don't merge - HAS MERGE CONFLICTS label May 14, 2019

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label May 14, 2019

DubeySandeep added 4 commits May 15, 2019 01:57

Merge branch 'develop' of https://github.com/oppia/oppia into full-co…

b90d278

…deowners

Added codeowners to new files.

04bbe6e

Added proper comment as suggested by reviewer.

97e66d8

Merge branch 'develop' of https://github.com/oppia/oppia into full-co…

b59ecce

…deowners

DubeySandeep merged commit 3d2a7fa into oppia:develop May 16, 2019

NishealJ pushed a commit to NishealJ/oppia that referenced this pull request May 21, 2019

Full coverage for CODEOWNERS in the codebase. (oppia#6616)

56874e3

		# Skill project.
		/core/controllers/concept_card_viewer*.py @aks681

Full coverage for CODEOWNERS in the codebase. #6616

Full coverage for CODEOWNERS in the codebase. #6616

Conversation

DubeySandeep commented Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

seanlip Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 17, 2019 • edited by codecov bot Loading

Codecov Report

DubeySandeep commented Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 17, 2019

apb7 commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep commented Apr 26, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashJipkate left a comment

Choose a reason for hiding this comment

DubeySandeep commented Apr 26, 2019

DubeySandeep commented Apr 29, 2019

oppiabot bot commented Apr 29, 2019

YashJipkate commented Apr 29, 2019

YashJipkate left a comment

Choose a reason for hiding this comment

DubeySandeep commented May 5, 2019

DubeySandeep commented May 8, 2019

DubeySandeep commented May 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep commented Apr 17, 2019 •

edited

Loading

seanlip Apr 17, 2019 •

edited

Loading

codecov-io commented Apr 17, 2019 •

edited by codecov bot

Loading

DubeySandeep commented Apr 17, 2019 •

edited

Loading