Skip to content
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

Notes functionality added for every Job #283

Merged
merged 1 commit into from
Sep 14, 2015

Conversation

deshraj
Copy link
Contributor

@deshraj deshraj commented Sep 9, 2015

Now, notes can be added for every job. Also, the user can update the notes. Let me know if any changes are required.

@deshraj
Copy link
Contributor Author

deshraj commented Sep 9, 2015

The build has failed because /home/travis/build/NVIDIA/DIGITS/docs/FlaskRoutes.md needs to be regenerated. @lukeyeager , please let me know if I need to change something to make it run successfully or it needs to be configured from the server side.

@@ -132,6 +133,9 @@ def path_is_local(self, path):
def name(self):
return self._name

def notes(self):
return self._notes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a check to make sure that _notes exists and return None if not. For older jobs I'm getting this error:

AttributeError: 'ImageClassificationModelJob' object has no attribute '_notes'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have added the condition now. Please check the condition.

@lukeyeager
Copy link
Member

@deshraj this is great work, thanks!

TravisCI
You need to run scripts/generate_docs.py. Make sure you run pip install -r requirements_test.txt first to get the latest version of Flask-Autodoc.

Tests
Please add a test for this. Props if you add one for editing job names too (I forgot to do that).

CLA
I'll need you to send us a signed CLA before I can accept your contribution.

Send a signed copy of the Contributor License Agreement (CLA) to digits@nvidia.com.

- https://github.com/NVIDIA/DIGITS/blob/master/CONTRIBUTING.md#pull-requests

@lukeyeager
Copy link
Member

@jmancewicz, how do you feel about the placement for the notes? I don't have a problem with it - it's no worse than the rest of the layout.

@@ -132,6 +133,12 @@ def path_is_local(self, path):
def name(self):
return self._name

def notes(self):
if self._notes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually doesn't help. You want this:

if hasattr(self, '_notes'):

@deshraj
Copy link
Contributor Author

deshraj commented Sep 9, 2015

Ok sure, I will add the test for editing the name also. :)

@deshraj
Copy link
Contributor Author

deshraj commented Sep 10, 2015

@lukeyeager , I have successfully sent the CLA. Also, I have updated the docs. Now, its all working.

@lukeyeager
Copy link
Member

I see that, thanks!

  1. You said you were going to add some tests - do you need some help?
  2. Before merging, I'll need you to squash this all into a single commit.
git remote update
git rebase -i origin/master
# squash all but the first commit

@deshraj
Copy link
Contributor Author

deshraj commented Sep 12, 2015

@lukeyeager , Is it right to rebase the commits that are already pushed ? Its not recommended to rebase the commits that are pushed. Please let me know if I am wrong. :)

@lukeyeager
Copy link
Member

Squashing
It's a question of style, really. I don't think that a change this small requires 8 commits. Squashing commits in a feature branch before merging them is perfectly fine.
You're probably thinking of the rule that public branches should never have their commit history rewritten. But a pull request branch doesn't count as a "public branch."

Tests
You didn't actually add any tests yet - only some helper stubs for tests. You can check to see if the total number of tests increased.
If you can't figure it out, don't worry about it. I'll write some tests for it after we merge this.

@deshraj
Copy link
Contributor Author

deshraj commented Sep 14, 2015

Yeah, I was thinking about the public branches rule but now it seems fine. Ok I will do this as soon as possible.

For the tests part: I think I need some help for that. Can you provide me some link for demo tests and the file where I need to write the tests ?

notes attribute renamed as _notes

Adding and updating notes functionality added in jobs

route created for updating the notes of a particular job using PUT request

Bug Fix: Check condition added to provide support to previous exisiting jobs

Fix: Check conditon updated for previously existing jobs

New Docs generated
@deshraj
Copy link
Contributor Author

deshraj commented Sep 14, 2015

Hey @lukeyeager , can you merge the pull request and write the test cases yourself. I am finding it somewhat difficult and time taking.

@lukeyeager
Copy link
Member

Hey @lukeyeager , can you merge the pull request and write the test cases yourself. I am finding it somewhat difficult and time taking.

Sure, no problem. Thanks for squashing, and for the great work!

lukeyeager added a commit that referenced this pull request Sep 14, 2015
Notes functionality added for every Job
@lukeyeager lukeyeager merged commit 85f6147 into NVIDIA:master Sep 14, 2015
lukeyeager added a commit to lukeyeager/DIGITS that referenced this pull request Sep 14, 2015
lukeyeager added a commit to lukeyeager/DIGITS that referenced this pull request Sep 14, 2015
@deshraj deshraj mentioned this pull request Sep 14, 2015
lukeyeager added a commit to lukeyeager/DIGITS that referenced this pull request Sep 16, 2015
lukeyeager added a commit to lukeyeager/DIGITS that referenced this pull request Sep 17, 2015
@jmancewicz
Copy link
Contributor

I agree, the location is a bit out of the way, but not worse than what else is going on. It may be moved around in the not too distant future.

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

Successfully merging this pull request may close these issues.

3 participants