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.

Sign up for GitHub

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

Jump to bottom

Complete roundtrip of PL <--> OPB MD conversion #87

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Complete roundtrip of PL <--> OPB MD conversion #87

wants to merge 30 commits into from

Conversation

Bluesy1
Copy link
Collaborator

@Bluesy1 Bluesy1 commented Aug 11, 2023 •
edited
Loading

Complete the PL to/from OPB MD roundtrip

TODO:

  • Add support for asset files
  • Reorganize new imports to their own section instead of at the top of the import list
  • Add parameter for output destination and write file(s) generated to destination
    • Also copy back asset files
  • Add test cases (expected outputs -> markdown files)
  • Update test action to upload generated return trip outputs
  • Add documentation elsewhere in the file to note cases when new features are added will require the parts of the parser to be updated to match compatibility
  • Add support for keys that cannot be roundtripped (waiting on progress on Finish the roundtrip: PL --> MD conversions #86 for this)

Resolves #86

@Bluesy1
First pass at parsing prairielearn question files
16bdafb 
Does not try to retain assets, or defined asset values, and loses the types of non string customizations currently.
@Bluesy1 Bluesy1 self-assigned this Aug 11, 2023
@Bluesy1
Expand implementation
f1ddfe2 
- Add missing section to md result we cannot roundtrip right now
- Move imports to a dedicated sections with a comment
- Using `ast.literal_eval`, customizations that are literals are parsed back from strings to their native type
@Bluesy1
Use inspect over code obj internals where possible
945d294
@Bluesy1
Add basic asset handling
4858bc9 
now attempts to list assets found in the 4 key folders from prairielearn
@Bluesy1
Adjust inspect usage from 945d294
27723fd 
Need to flatten the namedtuple to a list
@Bluesy1
Expand pl_to_md implementation
ffe2818 
- Now copies asset files
- Allows for an output directory and filename to be passed
- Validates input paths are directories and not files
- Improves argument checking
@Bluesy1
Copy link
Collaborator Author

Bluesy1 commented Aug 12, 2023

Benchmark of what output looks like as of August 11th, 2023: https://gist.github.com/Bluesy1/6f497bc1ed5b4a3aae25f91918abe2ad

@Bluesy1
Fix implementation based on testing
7f3d995 
- Need to prevent `"..."` from being evaled to the `Ellipses` literal
- Remove the `__pycache__` directory created by importing and executing the `server.py` file
- Fix `server.py` parsing
- Fix asset copying
- Add `pl-rich-text-editor` to list of supported pl inputs
@Bluesy1
Add tests
ad373ef 
Also minor updates to overall test layout to make round trip testing work nicer
@Bluesy1
Merge branch 'main' into parse-questions-to-markdown
6777d87
@Bluesy1
Update tests action
4badf4e 
Now should upload md generated from pl files on failure
@Bluesy1
Add comments, update pl_to_md impl, more tests
fc79dbf
@Bluesy1
Remove accidentally committed test files
4f70d46
@Bluesy1
Cleanup Formatting
0519f9a
@Bluesy1
Update pbs to write extra keys needed for roundtrip to object inside …
c293cd9 
…metadata field of the comment object in `info.json`
@Bluesy1
Update tests
1dc545c
@Bluesy1 Bluesy1 marked this pull request as ready for review August 18, 2023 02:56
@Bluesy1 Bluesy1 requested a review from firasm August 18, 2023 03:00
@Bluesy1
Copy link
Collaborator Author

Bluesy1 commented Aug 18, 2023

For now, to make the roundtrip possible, I've shoved all the extra keys I can't get normally into a nested object inside the comment section of the info.json generated files to at least see how that would look - I'm happy to change and/or modify the method if needed, or just accept that those sections have to be manually copied back (reverted to prior state?), by a human.

I expect this will need multiple rounds of review since its a fairly large change, but it should be mostly feature complete now and working.

@Bluesy1 Bluesy1 changed the title [WIP] Complete roundtrip of PL <--> OPB MD conversion Complete roundtrip of PL <--> OPB MD conversion Aug 18, 2023
@Bluesy1 Bluesy1 mentioned this pull request May 27, 2024
Merged
@Bluesy1 Bluesy1 marked this pull request as draft June 3, 2024 00:48
@Bluesy1 Bluesy1 marked this pull request as ready for review June 3, 2024 02:22
@Bluesy1 Bluesy1 force-pushed the main branch 2 times, most recently from 5d27a03 to 5e18acf Compare June 19, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firasm firasm Awaiting requested review from firasm

Assignees

@Bluesy1 Bluesy1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Finish the roundtrip: PL --> MD conversions
1 participant
@Bluesy1