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

Command install activate nbgrader extension. #168

Merged
merged 16 commits into from
Mar 26, 2015
Merged

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Mar 21, 2015

Rip off from jupyter-drive installation for now.
Need some modifications.

@jhamrick
Copy link
Member

Awesome! Though maybe it would be more consistent for this to go in nbgrader.apps.nbgraderapp.py as an additional subcommand?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.03%) to 93.46% when pulling 9439122 on Carreau:activate into 288a1d5 on jupyter:master.

@Carreau
Copy link
Member Author

Carreau commented Mar 21, 2015

Awesome! Though maybe it would be more consistent for this to go in nbgrader.apps.nbgraderapp.py as an additional subcommand?

I was feeling that this was more a once in a blue moon thing, that you probably don't want to run everyday, and that only admins run, so pretty hidden.

Up to you.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.03%) to 93.46% when pulling 7dcfdf7 on Carreau:activate into 288a1d5 on jupyter:master.

@Carreau
Copy link
Member Author

Carreau commented Mar 22, 2015

python -m nbgrader --activate default works now. Symlink by default :-)

@Carreau
Copy link
Member Author

Carreau commented Mar 25, 2015

I think I'll give up on installing the nbextension in the temporary folder created in test_nbextension...

@jhamrick
Copy link
Member

I think that's fine. Probably the test doesn't need to actually create a temporary directory -- I'm not entirely sure why I had it do that, so I think it would be fine if you just removed that functionality if you wanted to.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.57%) to 93.92% when pulling 921c87f on Carreau:activate into 288a1d5 on jupyter:master.

@@ -1,7 +1,10 @@
import subprocess as sp
import tempfile
import os
import os.path
Copy link
Member

Choose a reason for hiding this comment

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

I think this import is redundant

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, sys also I guess. I need to clean a bit...

@Carreau
Copy link
Member Author

Carreau commented Mar 25, 2015

cov <installscript> was not working, I removed it in favor a actually calling main(<arglist>) in install nbextension tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.23%) to 94.26% when pulling 554eb80 on Carreau:activate into 288a1d5 on jupyter:master.

@Carreau
Copy link
Member Author

Carreau commented Mar 25, 2015

o_O that's a bit scary how things wrk or not... if I do the following with data_files:

/
/nbgrader/
/nbextensions/

And move things around at install time, it should work on user machine.

Though for development, If you python setup.py install, PY3 is smart enough to "link" to the cloned repo and so the layout of file it sees is not the same that to one that would make the wheels or be installed...

So you cannot run things fro inside dev dir and/or you have to special case in code ...

3rd possibility, is I do smth really wrong.

I think I'll revert to move the nbextension data files in package tomorrow.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 93.41% when pulling 71431c9 on Carreau:activate into a7e54f7 on jupyter:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 93.4% when pulling 71431c9 on Carreau:activate into a7e54f7 on jupyter:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 93.4% when pulling 3a56792 on Carreau:activate into a7e54f7 on jupyter:master.

@jhamrick
Copy link
Member

Ok, I think this is working the way it's supposed to -- I guess the only other thing would be to implement the deactivation part? I can do that in a bit, too.

@jhamrick
Copy link
Member

Ok @Carreau you should take a look over this -- is there anything else you wanted to do with it? If not, then let me know, and I'll merge it!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.27%) to 93.44% when pulling ab2c80a on Carreau:activate into a7e54f7 on jupyter:master.

@Carreau
Copy link
Member Author

Carreau commented Mar 25, 2015

Ahhhhh the comma after install.... why did I missed it ! Will have a look.

if 'nbgrader/create_assignment' not in config['load_extensions']:
return

config['load_extensions']['nbgrader/create_assignment'] = False
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this will deactivate the ext. Actually the JS config use a dict as a set, but set do not exist in json, the value dont' have effet yet. You we might have to delete the key.
Let me check.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, ok, good catch.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case, then probably the tests should be updated too to make sure the key doesn't exist rather than checking for False.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.
Le 25 mars 2015 à 15:03, Jessica B. Hamrick notifications@github.com a écrit :

In nbgrader/install.py:

  • json_dir = os.path.join(pdir, 'nbconfig')
  • json_file = os.path.expanduser(os.path.join(json_dir, 'notebook.json'))
  • try:
  •    with io.open(json_file, 'r') as f:
    
  •        config = json.loads(f.read())
    
  • except IOError:
  •    # file doesn't exist yet. IPython might have never been launched.
    
  •    return
    
  • if 'load_extensions' not in config:
  •    return
    
  • if 'nbgrader/create_assignment' not in config['load_extensions']:
  •    return
    
  • config['load_extensions']['nbgrader/create_assignment'] = False
    If that's the case, then probably the tests should be updated too to make sure the key doesn't exist rather than checking for False.


Reply to this email directly or view it on GitHub.

Key should be removed and not set to false to deactivate
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.29%) to 93.42% when pulling c37106b on Carreau:activate into a7e54f7 on jupyter:master.

@Carreau
Copy link
Member Author

Carreau commented Mar 25, 2015

Yeahhhh works !

@jhamrick I'll let you push Little Greeny

@jhamrick
Copy link
Member

Woohoo! 🎉 🍰 Thanks @Carreau !

jhamrick added a commit that referenced this pull request Mar 26, 2015
[Work in progress] Command install activate nbgrader extension.
@jhamrick jhamrick merged commit 6c862b2 into jupyter:master Mar 26, 2015
@Carreau
Copy link
Member Author

Carreau commented Mar 26, 2015

Hey ! Go take care of your relatives !

@Carreau Carreau deleted the activate branch March 26, 2015 03:01
@jhamrick
Copy link
Member

Doing that now :)

@jhamrick jhamrick changed the title [Work in progress] Command install activate nbgrader extension. Command install activate nbgrader extension. Mar 26, 2015
@jhamrick jhamrick modified the milestone: 1.0 Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants