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.

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

Developers documentation #1389

Merged
merged 8 commits into from Jul 9, 2015

Conversation

Projects
None yet
3 participants
@aashish24
Contributor

aashish24 commented Jun 13, 2015

No description provided.

aashish24 added some commits Jun 8, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 13, 2015

Not ready for merge yet.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jun 13, 2015

@doutriaux1 please review it. If there are no major issues then I would like to get this merged soon.

@aashish24 aashish24 force-pushed the developers_documentation branch from ed391b8 to ebbcc8d Jun 13, 2015

Feel free to ask questions on [mailing
list](uvcdat-users@llnl.gov)

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

@aashish24 please mention askbot here too

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Sure, I will

git branch shiny-new-feature
git checkout shiny-new-feature

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

should we add a line here to make it track the branch back on your repo?

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Not sure what you meant? Can you elaborate?

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

see my comment bellow where you explain "git push"

*UV-CDAT* uses the [PEP8](http://www.python.org/dev/peps/pep-0008/)
standard. There are several tools to ensure you abide by this standard.

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

links? names?

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

should we mention that pep8 comes with UV-CDAT?

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

No, I think that's important I think. I will see if I can add reference to tools.

checking the style of your code. Additional standards are outlined on
the [code style wiki
page](LINK HERE).

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

missing link

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

should we add this to UV-CDAT if we mention it? Otherwise we might want to take it out?

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

I think we should. I just left it like that for the future but I can take it out for now.

time so this is never an issue.
#### Writing tests

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

You need to add a section on editing the CMakeLists.txt files

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Sure, Can I do it another PR? Once the other branch goes it (for testing), I can add more detail to it (how to get sample data, where to edit CMakeLists etc).

package. There are probably many examples already there and looking to
these for inspiration is suggested.
The `testing.checkimage.py` module has special `check_result_image` function

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

would stick this in a sub section "Images Checking"

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Sure, I was thinking for later. Currently most of the tests are regression testing so no need for separate section. But once we have more standard test mechanism, we can move them into sub-sections.

#### Running the test suite
The tests can then be run directly inside your build tree by typing::

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

quick reminder to the user what the "build" tree is?

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Not sure how do you want to refer it?

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

maybe just "build directory" rather than "build tree" ?

#### Running the performance test suite
TODO

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

do we even have a "performance" test suite yet? Let's not put this section in yet

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Not yet, we will though. Will take out

### Documenting your code
TODO

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

Might want to add this near/above the pep8 section

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Hmm..not sure since this is more general than just pep8

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

yes... maybe group both into a single section "UV-CDAT Coding Practices" or something like that. Basically I think this should come before/near the pep8 bit.

When you want your changes to appear publicly on your GitHub page, push
your forked feature branch's commits :
git push origin shiny-new-feature

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

see, it's probably better we mention this in the section where they create the branch, hence my "tracking comment" above.

This will automatically update your Pull Request with the latest code
and restart the Travis-CI tests.
### Delete your merged branch (optional)

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

"from your repo"

This comment has been minimized.

@aashish24

aashish24 Jun 15, 2015

Contributor

Its not from their repo necessarily. It could be deleting a local branch as well.

This comment has been minimized.

@doutriaux1

doutriaux1 Jun 15, 2015

Member

Well locally we don't really care, mostly you want the repo clean. If it's clean a git fetch -p will clean locally. No? What I really meant was that if it's directly on the UV-CDAT repo we will take care of deleting the branch (from the repo) but we can't for forks.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jun 15, 2015

@aashish24 that is great! Thanks for this. I made a few comments. Please review them and let me know.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 9, 2015

@doutriaux1 @sankhesh please review.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 9, 2015

I addressed most of the concerns.

doutriaux1 added a commit that referenced this pull request Jul 9, 2015

@doutriaux1 doutriaux1 merged commit 98a4e88 into master Jul 9, 2015

0 of 3 checks passed

continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build started
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@doutriaux1 doutriaux1 deleted the developers_documentation branch Jul 9, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 10, 2015

thanks @doutriaux1

@remram44

This comment has been minimized.

Contributor

remram44 commented on CONTRIBUTING.md in 2378fcc Jul 10, 2015

Mailing-list link is dead

@remram44

This comment has been minimized.

Contributor

remram44 commented on CONTRIBUTING.md in 08bc2bb Jul 10, 2015

This doesn't make sense

@remram44

This comment has been minimized.

Contributor

remram44 commented on CONTRIBUTING.md in ebbcc8d Jul 10, 2015

There is no need to enable this, pull requests will always be tested by Travis since it is enabled in this repo.

Enabling it on a fork only makes it build branches before a PR is opened.

@remram44

This comment has been minimized.

Contributor

remram44 commented on DEVELOP.md in 9fe5b10 Jul 10, 2015

A fork's master branch doesn't matter, changes can very well go there. Doesn't warrant a warning IMHO.

@remram44

This comment has been minimized.

Contributor

remram44 commented on DEVELOP.md in 395a212 Jul 10, 2015

This is not a module name

This comment has been minimized.

Member

doutriaux1 replied Jul 10, 2015

well... You do "import" it, so for simplicity I guess @aashish24 was right here

This comment has been minimized.

Contributor

remram44 replied Jul 10, 2015

The module name is testing.checkimage (or even checkimage, I don't know if testing is a package?)

This comment has been minimized.

Member

doutriaux1 replied Jul 10, 2015

testing is a directory.. Python is pretty blurry about all these
see bellow we do:

 + pth = os.path.join(os.path.dirname(__file__),"..")
+ sys.path.append(pth)
+ import checkimage 

This comment has been minimized.

Contributor

remram44 replied Jul 10, 2015

I don't think the .py suffix is ever accepted though

This comment has been minimized.

Member

doutriaux1 replied Jul 10, 2015

Ah! I see your point now... You're right.

@remram44

This comment has been minimized.

Contributor

remram44 commented on DEVELOP.md in 395a212 Jul 10, 2015

This is not the syntax for Python comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment