Apply folders by features to part of components/ folder #6863
Conversation
YashJipkate
requested review from
aks681,
AllanYangZhou,
ankita240796,
apb7,
bansalnitish,
brianrodri,
DubeySandeep,
kevinlee12,
nithusha21,
seanlip,
vinitamurthi and
vojtechjelinek
as code owners
YashJipkate
changed the title
There was a problem hiding this comment.
Looks good, thanks @YashJipkate, just a few comments.
I think now is a good time to email oppia-dev@ to let them know what's going on. This is going to cause a bunch of merge conflicts.
Also, feel free to make other PRs of this form in parallel (on different branches) -- it's fine to have more than one open at a time.
| /core/templates/dev/head/components/background/ @ankita240796 | ||
| /core/templates/dev/head/components/embed_modal/ @ankita240796 | ||
| /core/templates/dev/head/components/loading/ @ankita240796 | ||
| /core/templates/dev/head/components/common-layout-directives/common-elements/alert-message.directive.ts @ankita240796 |
There was a problem hiding this comment.
You don't need to do this file by file -- it's hard to maintain. Instead, it's fine to assign someone to a directory, and in fact, hopefully what you're doing with the frontend refactoring helps to streamline this. So, please try to use directories where possible.
There was a problem hiding this comment.
But there are many files in a directory that do not have the same CODEOWNER. For example in button-directives/ the create-activity button, embed, and hint-and-solution belong to different CODEOWNERS.
There was a problem hiding this comment.
Yep, in that case, you can use individual filepaths. I'm only talking about cases where every file in a directory has the same codeowner.
There was a problem hiding this comment.
Yes I have included the directories where all the CODEOWNERS match (e.g. common-layout-directives/navigation-bars). Only those with mismatching CODEOWNERS are named by filepaths
| @@ -0,0 +1,57 @@ | |||
| // Copyright 2014 The Oppia Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
I see an addition here but no deletion?
There was a problem hiding this comment.
Deleted the file. It somehow didn't move but copied.
Done!
scripts/pre_commit_linter.py
Outdated
| @@ -1397,12 +1397,12 @@ def _check_js_and_ts_component_name_and_count(self): | |||
| not filepath.endswith('App.ts'))] | |||
| failed = False | |||
| summary_messages = [] | |||
| component_name = '' | |||
| # component_name = ''. | |||
There was a problem hiding this comment.
Don't comment out code; delete it if it's not being used.
You can always get it back from the source history.
Codecov Report
@@ Coverage Diff @@
## develop #6863 +/- ##
========================================
Coverage 94.99% 94.99%
========================================
Files 371 371
Lines 50628 50628
========================================
Hits 48094 48094
Misses 2534 2534
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## develop #6863 +/- ##
========================================
Coverage 94.99% 94.99%
========================================
Files 371 371
Lines 50628 50628
========================================
Hits 48094 48094
Misses 2534 2534
Continue to review full report at Codecov.
|
|
@seanlip The checks now pass. PTAL! |
Explanation
This PR applies folders by feature structure to a part of the components folder. More specifically it applies the concept to the following:
Checklist
python scripts/pre_commit_linter.pyandbash scripts/run_frontend_tests.sh.