Update the development guide doc #124

allieychen · 2018-02-27T18:05:40Z

Update the development guide doc.

Add IntelliJ IDE setup.
Add more details.

coveralls · 2018-02-27T18:11:34Z

Pull Request Test Coverage Report for Build 367

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 90.335%

Totals
Change from base Build 359:	0.0%
Covered Lines:	3075
Relevant Lines:	3404

💛 - Coveralls

arostamianfar

Thanks, Allie! Added a few comments. I'm excited to try it out on my own machine when I get back :)
Also, could you please edit the title of the pull request to something like "Update the development guide doc."?

arostamianfar · 2018-02-27T18:21:39Z

docs/development_guide.md

-to create your own fork of the repository. See
-[https://guides.github.com/activities/forking/](https://guides.github.com/activities/forking/)
-for more information.
+Visit the [gcp-variant-transforms repository](https://github.com/googlegenomics/gcp-variant-transforms) to create your own fork of the repository. See [https://guides.github.com/activities/forking/](https://guides.github.com/activities/forking/)


Please keep each line <80 chars. Links that are longer than 80chars should be in separate lines.
There is a google style guide on .md docs somewhere as well (i'll ping you when I've found it).

arostamianfar · 2018-02-27T18:27:17Z

docs/development_guide.md

-If you want to pull in changes from the target branch (i.e. googlegenomic:master),
-run:
+To update the forked repository, you may follow instructions on [syncing a fork](https://help.github.com/articles/syncing-a-fork/), or simply set upstream to the main repository and do:
+```bash


Do you need this section? This is the same as the instructions below except it doesn't have --rebase (and I think we needed to always pull with rebase for things to work smoothly).

I am thinking there are two scenarios.
Before checking out a new branch from the forked master branch, we would like to keep the forked master branch to be synced with the remote master branch, so that the commits from other people will apply. In that case, we update the forked master repository.
The second portion is after we made changes to local branch, we pull in the latest commits, then replay our local changes.
But you are right, the second portion is sufficient for the sync. I will remove this part.

Keep each line <=80 chars.

arostamianfar

Thanks! Looks good! I'll let Bashir comment on the IntelliJ setup :)

arostamianfar · 2018-02-27T20:49:49Z

docs/development_guide.md

-Visit the
-[gcp-variant-transforms repository](https://github.com/googlegenomics/gcp-variant-transforms)
-to create your own fork of the repository. See
+Visit the 


nit: please remove trailing space from here and elsewhere.

1. Remove trailing whitespace. 2. Add newline before and after headings.

Remove one more trailing whitespace.

bashir2

Thanks for documenting how to use IntelliJ.

bashir2 · 2018-02-28T22:23:24Z

docs/development_guide.md

+environment, and specify the location of the new virtual environment. Note that
+the folder where the new virtual environment should be located must be empty!
+For the Base interpreter, add the python path
+gcp-variant-transforms/venv/bin/python under the created virtualenv.


I put my venv under gcp-variant-transforms but I think some people don't (@arostamianfar do you do this?). So maybe replace "gcp-variant-transforms" with [PATH_TO_VENV] and mention that this 'venv' directory is what we created in "Setup virtualenv" above.

bashir2 · 2018-02-28T22:25:47Z

docs/development_guide.md

+
+To comply with pylint coding style, you may change the default line length in
+File | Settings | Editor | Code Style. Set the hard wrap at 80 columns and
+check Wrap on typing. Further, go to Python in the dropdown list, you can


I have not done this, how annoying is it when you want to have a line over 80? (e.g., some of our "# pylint" comments for imports are like this).

I am not aware of a better solution other than manually adjust the line <= 80 without setting this. I would say it would be more annoying without it since it happens more often than the other case.

bashir2 · 2018-02-28T22:27:34Z

docs/development_guide.md

+```
+Then run:
+```bash
+pylint gcp_variant_transforms


Hmm, I think you should use the .pylintrc file that is checked into the git repo not the default RC file. I usually run pylint --rcfile=.pylintrc gcp_variant_transforms/

Sorry, I tried both and it seems that they are doing the same checking. Does it mean that the default file is .pylintrc?

bashir2 · 2018-02-28T22:28:51Z

docs/development_guide.md

+
+Before pushing changes, make sure the pylint checks pass. To install pylint:
+```bash
+sudo apt-get install pylint


Did you successfully use the pylint that comes with your linux distribution? I had problems with mine originally (Trusty at the time) and I tried again on Rodete but it gives me a lot of warnings (it is an older version). I use a local pylint in my venv: pip install --upgrade pylint

Yes. I tried pip install --upgrade pylint in the first place and somehow it did not work.

So please update as we discussed.

bashir2 · 2018-02-28T22:34:01Z

docs/development_guide.md

 To commit and push those changes to your branch.

 ### Syncing your branch
+


Are these blank lines needed? If not, consider removing them here and elsewhere.

I referred to g3doc style guide, and it suggests newlines before and after the heading. If we do differently, I can remove it.

Oh, I see, then maybe consider updating it for every section in this doc.

bashir2 · 2018-02-28T22:36:02Z

docs/development_guide.md

+
+#### Setup IntelliJ SDK
+
+1. Choose File | Project Structure on the main menu, and then go to Project.


Please add an initial step on how the project is created. IIRC, the project could have been created in the default location but then there was somewhere that sources were being set, correct?

Done. Thanks.

bashir2 · 2018-02-28T22:39:21Z

docs/development_guide.md

+File | Settings | Editor | Inspections.
+
+Code inspections can be run from the Analyze menu. The result window can be
+accessed from View > Tool Windows.


Should you add any comments about code inclusion/exclusion? I have excluded everything other than gcp_variant_transforms (see screenshot) and IIRC this was important to trim the long list of inspection warnings.

allieychen · 2018-03-01T15:55:32Z

PTAL :)

Add create project section and details on code inspection.

allieychen · 2018-03-01T20:05:42Z

I edited the --amend part.

bashir2 · 2018-03-01T20:13:00Z

docs/development_guide.md

+
+To comply with pylint coding style, you may change the default line length in
+File | Settings | Editor | Code Style. Set the hard wrap at 80 columns and
+check Wrap on typing. Further, go to Python in the dropdown list, you can


bashir2 · 2018-03-01T20:17:36Z

docs/development_guide.md

+
+Before pushing changes, make sure the pylint checks pass. To install pylint:
+```bash
+sudo apt-get install pylint


So please update as we discussed.

bashir2 · 2018-03-01T20:17:50Z

docs/development_guide.md

+```
+Then run:
+```bash
+pylint gcp_variant_transforms


bashir2 · 2018-03-01T20:18:58Z

docs/development_guide.md

 To commit and push those changes to your branch.

 ### Syncing your branch
+


Oh, I see, then maybe consider updating it for every section in this doc.

bashir2 · 2018-03-01T20:22:44Z

docs/development_guide.md

+After making changes, you must again add, commit, and push those changes.
+If you are making changes after a review process, you may create new commits for
+related changes by following the steps stated in "Pushing changes to your fork's
+branch". Otherwise, please run the following:


Please add a note like: "Prefer to have one commit per each review round such that your reviewers can easily check what you have changed since last time."

bashir2 · 2018-03-01T20:25:41Z

docs/development_guide.md

+#### Create Project
+
+Choose File | New | Project on the main menu, and create a new Python project
+in the dialog that opens. To setup the Project SDK, follow the following steps.


Please add a step before this which says where Project SDK is, i.e., from File | Project Structure go to the Project sub-menu ...

That is one way to setup the SDK. Also, we can setup it directly in the new project dialog.

allieychen · 2018-03-01T21:55:02Z

PTAL :)

bashir2 · 2018-03-02T18:19:02Z

docs/development_guide.md

+#### Create Project
+
+Choose File | New | Project on the main menu, and create a new Python project
+in the dialog that opens. To setup the Project SDK, follow the following steps.


bashir2

The following comments are minor, please feel free to submit once you have addressed them.

bashir2 · 2018-03-02T18:23:00Z

docs/development_guide.md

+
+Before pushing changes, make sure the pylint checks pass. To install pylint:
+```bash
+.[PATH_TO_VENV]/bin/activate


You need a space after '.'; also in general I prefer source to using '.', i.e.,
source [PATH_TO_VENV]/bin/activate
as it is more clear what is being done.

Thanks. Done.

bashir2 · 2018-03-02T18:23:44Z

docs/development_guide.md

+Before pushing changes, make sure the pylint checks pass. To install pylint:
+```bash
+.[PATH_TO_VENV]/bin/activate
+pip install pylint


nit: consider adding --upgrade to be consistent with elsewhere.

1. replace . with source 2. add --upgrade for pylint installation

allieychen · 2018-03-02T18:57:29Z

Thanks, PTAL :)

arostamianfar

Thanks!

* Add multiple query support to integration tests googlegenomics#120. update all tests in integration small_tests to support multiple query. update the validate_table in run_tests, add a loop for all test cases in the test file. changed the required keys for the .json file. Remove "validation_query" and "expected_query_result", and add "test_cases". Ran ./deploy_and_run_tests.sh and all integration tests passed. Update the development guide doc (googlegenomics#124) Update the development guide doc. Add IntelliJ IDE setup. Add more details. Added an INFO message for the full command. Tested: Ran manually and checked the new log message. Uses the macros to replace the common queries. (googlegenomics#127) Define NUM_ROWS, SUM_START, SUM_END in QueryFormatter, and replaces them in the query to avoid duplicate code. TESTED: deploy_and_run_tests. Define SchemaDescriptor class. Provides serialization and lookup API for type/mode of schema fields. Tested: unit test

Update the development guide doc. Add IntelliJ IDE setup. Add more details.

allieychen added 2 commits February 27, 2018 11:59

Update development_guide.md

7cb7c8b

Update development_guide.md

5219329

allieychen requested review from arostamianfar and bashir2 February 27, 2018 18:06

arostamianfar suggested changes Feb 27, 2018

View reviewed changes

allieychen changed the title ~~Allieychen guide doc~~ Update the development guide doc Feb 27, 2018

allieychen added 2 commits February 27, 2018 14:52

Update the format

76b4301

Keep each line <=80 chars.

Update format

14f7f4a

arostamianfar reviewed Feb 27, 2018

View reviewed changes

allieychen added 2 commits February 28, 2018 09:46

Update development_guide.md

9b6d8ed

1. Remove trailing whitespace. 2. Add newline before and after headings.

Update development_guide.md

e1cba05

Remove one more trailing whitespace.

bashir2 reviewed Feb 28, 2018

View reviewed changes

Update development_guide.md

492f06e

Add create project section and details on code inspection.

allieychen force-pushed the allieychen-guide-doc branch from 264b7ba to 492f06e Compare March 1, 2018 20:04

bashir2 reviewed Mar 1, 2018

View reviewed changes

Correct pylint installation and running command

7dc37b6

allieychen force-pushed the allieychen-guide-doc branch from a40b872 to 7dc37b6 Compare March 1, 2018 21:53

Merge branch 'master' into allieychen-guide-doc

ffbed91

bashir2 approved these changes Mar 2, 2018

View reviewed changes

bashir2 previously approved these changes Mar 2, 2018

View reviewed changes

Update development_guide.md

c34d9f2

1. replace . with source 2. add --upgrade for pylint installation

allieychen dismissed bashir2’s stale review via c34d9f2 March 2, 2018 18:56

bashir2 approved these changes Mar 2, 2018

View reviewed changes

arostamianfar approved these changes Mar 2, 2018

View reviewed changes

allieychen merged commit 877eee8 into googlegenomics:master Mar 2, 2018

allieychen deleted the allieychen-guide-doc branch March 2, 2018 20:37

mhsaul pushed a commit to mhsaul/gcp-variant-transforms that referenced this pull request Mar 29, 2018

Update the development guide doc (googlegenomics#124)

e8a25d5

Update the development guide doc. Add IntelliJ IDE setup. Add more details.

mhsaul pushed a commit to mhsaul/gcp-variant-transforms that referenced this pull request Mar 29, 2018

Update the development guide doc (googlegenomics#124)

b2eab41

Update the development guide doc. Add IntelliJ IDE setup. Add more details.

		To commit and push those changes to your branch.

		### Syncing your branch


		#### Setup IntelliJ SDK

		1. Choose File \| Project Structure on the main menu, and then go to Project.

Update the development guide doc #124

Update the development guide doc #124

Uh oh!

Conversation

allieychen commented Feb 27, 2018

Uh oh!

coveralls commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 367

💛 - Coveralls

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bashir2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen Mar 1, 2018 • edited by arostamianfar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen commented Mar 1, 2018

Uh oh!

allieychen commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 27, 2018 •

edited

Loading

allieychen Mar 1, 2018 •

edited by arostamianfar

Loading