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

Adding global graded tasks #984

Merged
merged 51 commits into from Dec 17, 2018

Conversation

Projects
None yet
2 participants
@danielmaitre
Copy link
Contributor

commented Jun 30, 2018

Hi,
this is an attempt at adding a new type of cells: task cells. I described what I wanted to achieve in #977 . They are tasks one can set to the students that are not tied to a single cell. I think they are useful when entire sections of notebooks are considered and it allows to ask students to develop parts of a notebook and get grades for it. Task cell can be used for task where we want students to use more than one cell to submit an answer and in this way exercises using the notebook as a platform rather than a single cell.

I'm sorry it ended up being quite a large set of changes. Let me know what you think!

Daniel

danielmaitre added some commits Jun 12, 2018

fixed problem with submission query
(missing explicit select_from)
renamed tables to facilitate the alembic migration
it is apparently not possible to drop columns from tables in
sqlite so it is easier to move to a new table and drop the old
ones.
Added mark scheme
this is a section like hidden tests that gets removed from the assigned
notebooks but can be seen at the marking stage and is visible in
the feedback.
@jhamrick

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

Thanks so much for all the additional work on this PR, and sorry I've been slow to look at it. I'll take a closer look in the next few days!

@jhamrick

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Ok, I'm going to go through the changes over the next day or two (it might take me a bit as there is a lot to review) but in the meantime here are a few high-level things:

  • The PR has merge conflicts (though now the tests should run succesfully against master, hopefully), so would you mind doing a rebase?
  • Could I ask you to run pep8 on the files you've changed and make sure the style adheres? I've noticed there are a lot of places where there are e.g. spaces missing after a comma, wrong continued indentation, etc. I know this probably seems nitpicky but it really helps for readability to have the code written in the same style everywhere and to roughly follow pep8.
  • Regarding the drop down menu, yes, I think it would be better to only have the "task cell" option visible for markdown cells.
  • In the documentation, it's not immediately clear from the description what the distinction is between a manually graded answer cell versus a manually graded task. I would just add a sentence or two making it really explicit when you should use one versus the other.

Overall though this looks really nice; I'm really impressed you were able to figure out all the details and complexities of nbgrader in putting this together! It's clear you've put a lot of work into this so hopefully we can get it merged soon---sorry again that it's taken so much time.

@danielmaitre

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Hi, thank you for the comments! I will do the rebase and the PEP8 checks in a few days.

@danielmaitre

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

HI, I've made the changes as requested, sorry for the delay. As a side note I've used nbgrader with task cells for exercises for a class with 180 students and it worked very well!

@jhamrick
Copy link
Member

left a comment

This is looking really great. I just have a few small misc comments, then I think we can merge!

Show resolved Hide resolved nbgrader/nbextensions/create_assignment/main.js Outdated
Show resolved Hide resolved nbgrader/nbextensions/create_assignment/main.js Outdated
Show resolved Hide resolved nbgrader/nbextensions/create_assignment/main.js Outdated
Show resolved Hide resolved nbgrader/nbextensions/create_assignment/main.js Outdated
Show resolved Hide resolved nbgrader/nbextensions/create_assignment/main.js Outdated

@jhamrick jhamrick removed the needs review label Dec 15, 2018

jhamrick and others added some commits Dec 16, 2018

Apply suggestions from code review
Added @jhamrick suggestions

Co-Authored-By: danielmaitre <daniel.maitre@durham.ac.uk>
@jhamrick

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

Oops, tests look like they are passing but actually they are not, this is due to a bug introduced I think by #1053. Will hold off on merging until that's fixed, then we'll need to do another rebase/merge (sorry about that!)

@jhamrick

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

See #1054

Show resolved Hide resolved nbgrader/api.py Outdated
@jhamrick

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

Should be ready to rebase/merge with master again, and I think that suggested fix I gave should fix the tests!

danielmaitre and others added some commits Dec 17, 2018

Update nbgrader/api.py
according to jhamrick suggestion

Co-Authored-By: danielmaitre <daniel.maitre@durham.ac.uk>
@jhamrick

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Awesome, thanks!

@jhamrick jhamrick merged commit d56f75b into jupyter:master Dec 17, 2018

2 of 4 checks passed

codecov/patch 80.51% of diff hit (target 88.77%)
Details
codecov/project 87.49% (-1.29%) compared to bb609b2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jhamrick jhamrick referenced this pull request May 30, 2019

Closed

Global tasks #977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.