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

Assignment names breaking regex #1768

Closed
wants to merge 5 commits into from

Conversation

perllaghu
Copy link
Contributor

This extends the list of banned assignment-name characters beyond + to include a bunch that break the path regex code.

closes #1738

perllaghu and others added 5 commits March 3, 2023 14:06
Added a test for correct behavior

Adapts timeout in tests to avoid cancelled nbextension tests

In tests: (1) updates python (drop 3.7 and includes 3.11), and (2) update ubuntu from 20.04 to 22.04

Revert "Remove an errant commented line"

This removes the concept of replacing 'bad characters', and bans them

Revert "Change from excluding characters to swapping for high-order Unicode versions"

This actually does do the replace swap with ban thing
@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch edina/nbgrader/assignment_names_breaking_regex

@brichet
Copy link
Contributor

brichet commented Jun 12, 2023

Thanks @perllaghu.

I wonder if there could not be a way to manage special characters when running the regex match. Something like re.escape, to avoid constraining the assignment name.

@brichet
Copy link
Contributor

brichet commented Jun 12, 2023

On my side there is some confusion in this PR, I see some modifications already in the branch main (e.g. zero point validation).

@perllaghu
Copy link
Contributor Author

Thanks @perllaghu.

I wonder if there could not be a way to manage special characters when running the regex match. Something like re.escape, to avoid constraining the assignment name.

This is essentially an extension of the existing routine - which prevent + in assignment names.

@perllaghu
Copy link
Contributor Author

On my side there is some confusion in this PR, I see some modifications already in the branch main (e.g. zero point validation).

I'll rebase on master :)

@smith-kyle
Copy link

Just a heads up, you can review notebook changes like this using GitNotebooks.

e.g. this pull request https://gitnotebooks.com/jupyter/nbgrader/pull/1768

It's free for public repos, so might be helpful for y'all.

@lahwaacz
Copy link
Contributor

lahwaacz commented Feb 3, 2024

The last commit 88915ab is very confusing: it includes changes from 6706939 (in main), reverts 2 previous commits in this pull request and maybe also some unrelated changes (I can't tell at this point).

Please do an interactive rebase, drop the reverted commits, squash corrections into previous commits and overall make sure that this pull request can be properly reviewed.

@brichet
Copy link
Contributor

brichet commented May 20, 2024

This is essentially an extension of the existing routine - which prevent + in assignment names.

Agree, let's move forward with this PR and open an issue to allow these character in future development.
@perllaghu can you clean this PR please ?

@perllaghu
Copy link
Contributor Author

The sane move now will be to drop this & redo the work on a fresh master

@perllaghu perllaghu closed this May 21, 2024
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.

Assignments_ids with () in them fail in formgrader
5 participants