docs: added code review guide #1397

bhavsarpratik · 2020-12-04T09:12:49Z

No description provided.

codecov · 2020-12-04T09:30:48Z

Codecov Report

Merging #1397 (bdd53d2) into master (bf2d923) will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1397      +/-   ##
==========================================
+ Coverage   83.33%   83.47%   +0.13%     
==========================================
  Files         104      104              
  Lines        6860     6939      +79     
==========================================
+ Hits         5717     5792      +75     
- Misses       1143     1147       +4

Impacted Files	Coverage Δ
jina/types/sets/chunk_set.py	`91.66% <0.00%> (-8.34%)`	⬇️
jina/types/sets/match_set.py	`95.83% <0.00%> (-4.17%)`	⬇️
jina/logging/profile.py	`55.81% <0.00%> (-0.66%)`	⬇️
jina/types/document/__init__.py	`92.33% <0.00%> (-0.23%)`	⬇️
jina/peapods/pod.py	`83.18% <0.00%> (-0.10%)`	⬇️
jina/drivers/craft.py	`100.00% <0.00%> (ø)`
jina/types/ndarray/generic.py	`100.00% <0.00%> (ø)`
jina/executors/indexers/cache.py	`100.00% <0.00%> (ø)`
jina/executors/evaluators/text/length.py	`100.00% <0.00%> (ø)`
jina/executors/evaluators/rank/precision.py	`100.00% <0.00%> (ø)`
... and 23 more

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 fed9857...bdd53d2. Read the comment docs.

.github/CODE_REVIEW_GUIDELINES.md

alexcg1

Some tiny feedback to improve legibility

cristianmtr · 2020-12-04T14:03:28Z

.github/CODE_REVIEW_GUIDELINES.md

+- Add new unit & integration test if you add a new feature
+- Use pytest tmpdir fixture for temporary directories
+- Check test locally before pushing
+- If a `Flow()` needs to be built in the test and is not a test in the Flow module -> It belongs to integration test


Suggested change

- If a `Flow()` needs to be built in the test and is not a test in the Flow module -> It belongs to integration test

- If a `Flow()` needs to be built in the test and it's not a test in the Flow module, the test belongs under `/tests/integration`.

cristianmtr · 2020-12-04T14:03:45Z

.github/CODE_REVIEW_GUIDELINES.md

+
+**Interface segregation**  
+
+Do not create lengthy interfaces, instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.


Suggested change

Do not create lengthy interfaces, instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.

Do not create lengthy interfaces. Instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.

cristianmtr · 2020-12-04T14:04:33Z

.github/CODE_REVIEW_GUIDELINES.md

+
+**Dependency Injection**  
+
+Do not hardcode the dependencies, instead inject them.


Suggested change

Do not hardcode the dependencies, instead inject them.

Do not hardcode the dependencies; instead inject them.

cristianmtr · 2020-12-04T14:05:56Z

CONTRIBUTING.md

 1. Fork the Jina repo and clone onto your computer. By default, `git` won't clone `jina/hub` as it is a submodule maintained at [`jina-ai/jina-hub`](https://github.com/jina-ai/jina-hub). Please follow [the steps](#check-out-jina-hub-submodule) for details. 
 2. Create a [new branch](#naming-your-branch), for example `fix-jina-typo-1`.
 3. Work on this branch to do the fix/improvement.
-4. Commit the changes with the [correct commit style](#writing-your-commit-message).
-5. Make a pull request. 
-6. Submit your pull request and wait for all checks to pass.
-7. Request reviews from one of [the code owners](.github/CODEOWNERS).
-8. Get a LGTM 👍 and PR gets merged.
+4. Check if your code changes follow the [code review guidelines](.github/CODE_REVIEW_GUIDELINES.md).
+5. Commit the changes with the [correct commit style](#writing-your-commit-message).
+6. Make a pull request. 
+7. Submit your pull request and wait for all checks to pass.
+8. Request reviews from one of [the code owners](.github/CODEOWNERS).
+9. Get a LGTM 👍 and PR gets merged.


You can just write

1. First item 1. Second item

and you get automatic listing.

First item

Second item

So we avoid having changes to the numbering of the items after the one you add.

cristianmtr

Minor things

cristianmtr · 2020-12-04T14:18:23Z

CONTRIBUTING.md

@@ -64,14 +64,14 @@ Right now we're working on a list of things we want help with and easy-to-fix bu

 0. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).


Suggested change

0. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).

1. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).

It was originally kept 0 and so I let it be to give special importance. The rest of the numbering is working fine.

bwanglzu

great work, only this line Do not merge PR without approval. we can't do it without approval because the PR is blocked lol

Done

JoanFM · 2020-12-06T06:44:11Z

.github/CODE_REVIEW_GUIDELINES.md

@@ -13,13 +13,12 @@
 - The scope of PR should be simple, unique and well-defined. PR should not contain unrelated changes
 - Approve PR only if you are sure about the scope
 - Be respectful and reply asap
- Do not merge PR without approval
- Avoid spending too much time on trivial changes
+- Avoid spending time on trivial changes


Too much was right; I think some minor style changes quickly add up to ugly code

Didn't get you

alexcg1

LGTM 👍

alexcg1 · 2020-12-07T08:19:58Z

@hanxiao any feedback? Looks like GitHub waiting on you since you're code owner

* docs: added code review guide

docs: added code review guide

ba307e9

bhavsarpratik requested review from hanxiao and a team as code owners December 4, 2020 09:12

bhavsarpratik requested a review from yuanbit December 4, 2020 09:12

jina-bot added size/S area/housekeeping This issue/PR is housekeeping labels Dec 4, 2020

hanxiao requested a review from alexcg1 December 4, 2020 13:12

alexcg1 reviewed Dec 4, 2020

View reviewed changes

.github/CODE_REVIEW_GUIDELINES.md Outdated Show resolved Hide resolved

alexcg1 reviewed Dec 4, 2020

View reviewed changes

.github/CODE_REVIEW_GUIDELINES.md Outdated Show resolved Hide resolved

cristianmtr reviewed Dec 4, 2020

View reviewed changes

.github/CODE_REVIEW_GUIDELINES.md Outdated Show resolved Hide resolved

alexcg1 previously requested changes Dec 4, 2020

View reviewed changes

fix: typo

3a2d3bf

cristianmtr reviewed Dec 4, 2020

View reviewed changes

cristianmtr previously requested changes Dec 4, 2020

View reviewed changes

fix: typo

fe9d414

cristianmtr reviewed Dec 4, 2020

View reviewed changes

bwanglzu previously approved these changes Dec 5, 2020

View reviewed changes

docs: cleanup

31d970d

bhavsarpratik dismissed bwanglzu’s stale review via 31d970d December 6, 2020 04:11

JoanFM requested changes Dec 6, 2020

View reviewed changes

docs: cleanup

bdd53d2

JoanFM approved these changes Dec 7, 2020

View reviewed changes

alexcg1 approved these changes Dec 7, 2020

View reviewed changes

hanxiao approved these changes Dec 7, 2020

View reviewed changes

hanxiao merged commit ab078ea into master Dec 7, 2020

hanxiao deleted the docs-add-code-review branch December 7, 2020 09:21

harry-stark pushed a commit to harry-stark/jina that referenced this pull request Dec 7, 2020

docs: added code review guide (jina-ai#1397)

da87e30

* docs: added code review guide

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: added code review guide #1397

docs: added code review guide #1397

bhavsarpratik commented Dec 4, 2020

codecov bot commented Dec 4, 2020 •

edited

alexcg1 left a comment

cristianmtr Dec 4, 2020

cristianmtr Dec 4, 2020

cristianmtr Dec 4, 2020

cristianmtr Dec 4, 2020

cristianmtr left a comment

cristianmtr Dec 4, 2020

bhavsarpratik Dec 4, 2020

bwanglzu left a comment

JoanFM Dec 6, 2020 •

edited

bhavsarpratik Dec 6, 2020

JoanFM Dec 6, 2020

alexcg1 left a comment

alexcg1 commented Dec 7, 2020

	- If a `Flow()` needs to be built in the test and is not a test in the Flow module -> It belongs to integration test
	- If a `Flow()` needs to be built in the test and it's not a test in the Flow module, the test belongs under `/tests/integration`.


		Interface segregation

		Do not create lengthy interfaces, instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.

	Do not create lengthy interfaces, instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.
	Do not create lengthy interfaces. Instead split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.


		Dependency Injection

		Do not hardcode the dependencies, instead inject them.

	Do not hardcode the dependencies, instead inject them.
	Do not hardcode the dependencies; instead inject them.

		@@ -64,14 +64,14 @@ Right now we're working on a list of things we want help with and easy-to-fix bu

		0. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).

	0. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).
	1. Associate your local git config with your github account. If this is your first time using git you can follow [the steps](#associate-with-github-account).

docs: added code review guide #1397

docs: added code review guide #1397

Conversation

bhavsarpratik commented Dec 4, 2020

codecov bot commented Dec 4, 2020 • edited

Codecov Report

alexcg1 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

cristianmtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwanglzu left a comment

Choose a reason for hiding this comment

JoanFM Dec 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcg1 left a comment

Choose a reason for hiding this comment

alexcg1 commented Dec 7, 2020

codecov bot commented Dec 4, 2020 •

edited

JoanFM Dec 6, 2020 •

edited