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

Update codeblock answer substitution #69

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Conversation

Bluesy1
Copy link
Collaborator

@Bluesy1 Bluesy1 commented Jul 12, 2023

The changes here happened via cascade, I can split this into 4 separate PRs if wanted.

I recommend viewing the changes commit by commit, to understand what each is for:

Resolves #68

@Bluesy1 Bluesy1 added the bug Something isn't working label Jul 12, 2023
@Bluesy1 Bluesy1 self-assigned this Jul 12, 2023
@Bluesy1 Bluesy1 requested a review from firasm July 12, 2023 01:17
@firasm
Copy link
Contributor

firasm commented Jul 12, 2023

These three can be handled in the same PR:

  • fix to the unicode is fine, though I think we need to update our templates so there's a bit more complicated LaTeX in there so we don't keep missing weird edge cases (but this can be done in a later PR)
  • fix to the matching question is also fine
  • fix to question.html not being checked is also fine

  • codeblock substitution is probably what we need to discuss
    • I don't like relying on pbs to be installed in courses, I'm actually trying to get to a point where only pbh is installed in courses as it's a much simpler dependency. Maybe we can move pbs.backticks_to_code_tags to pbh? (wherever pbs is installed, pbh` will likely exist, but not vice versa)

We should have a meeting about this to hammer out some things as it'll require some back and forth.

@Bluesy1
Copy link
Collaborator Author

Bluesy1 commented Jul 12, 2023

  • I don't like relying on pbs to be installed in courses, I'm actually trying to get to a point where only pbh is installed in courses as it's a much simpler dependency. Maybe we can move pbs.backticks_to_code_tags to pbh? (wherever pbs is installed, pbh` will likely exist, but not vice versa)

The alternative I had thought of was to just move the body of the convert function into the injected code, and inject an re import instead of a pbs import - I just wasn't sure if that would be too messy, injecting those 12 lines of code in. Moving it to pbh is a valid alternative.

If you want to have a meeting to discuss the different options, I can be available on Thursday for that.

I'll go ahead and split the changes that aren't for the codeblocks into a different PR so those can be merged in separately since this may take some time to figure out the optimal solution for

Edit: #70 has the split off changes

@Bluesy1
Copy link
Collaborator Author

Bluesy1 commented Jul 12, 2023

I've rebased this off of #70 now, such that we can keep the changes in test running for anything made here, to be sure it works properly, but such that clean merging separately should hopefully be possible, though I suppose another rebase may be necessary down the line (should be relatively simple I think though)

@Bluesy1 Bluesy1 removed the bug Something isn't working label Jul 12, 2023
@firasm firasm changed the title Update codeblock answer substitution, fix pl-matching, tests, unicode change Update codeblock answer substitution Jul 12, 2023
Now, code is injected into `generate` to do the substitution at runtime
@Bluesy1 Bluesy1 force-pushed the update-codeblock-answers branch from 8e7c36a to 864a247 Compare July 26, 2023 00:00
@Bluesy1
Copy link
Collaborator Author

Bluesy1 commented Jul 26, 2023

Change implemented as discussed earlier, this should not be merged (or at least merged and released) until a new release of PBH with open-resources/problem_bank_helpers#17 merged is cut (and ideally the dependency is bumped here, but that doesn't matter as much)

@firasm firasm merged commit c5a11f8 into main Jul 26, 2023
@Bluesy1 Bluesy1 deleted the update-codeblock-answers branch April 9, 2024 13:49
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.

Support bacticks when answer options are created from a list, and other answer types
2 participants