Skip to content
This repository has been archived by the owner on Sep 25, 2019. It is now read-only.

fix(scripts): fix unpack and repack scripts for the new challenge schema #106

Closed

Conversation

gforce2k5
Copy link
Contributor

@gforce2k5 gforce2k5 commented Jul 3, 2018

ISSUES CLOSED: #42

Description

Pre-Submission Checklist

  • Your pull request targets the dev branch.
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/challenge-tests)
  • All new and existing tests pass the command npm test.
  • Use npm run commit to generate a conventional commit message.
    Learn more here: https://conventionalcommits.org/#why-use-conventional-commits
  • The changes were done locally on your machine and NOT GitHub web interface.
    If they were done on the web interface you have ensured that you are creating conventional commit messages.

Checklist:

  • Tested changes locally.
  • Addressed currently open issue (replace XXXXX with an issue no in next line)

Closes #42

@gforce2k5 gforce2k5 force-pushed the fix/unpack-and-repack-scripts branch from b4632e2 to 3eb0d2d Compare July 4, 2018 13:13
@scissorsneedfoodtoo
Copy link
Contributor

@gforce2k5, thank you for all your hard work on this. It will be great to have these scripts working again.

I did notice a few issues, though. Everything seems to run smoothly when unpacking. But after making changes and repacking, I get an error while running npm run test. Seems like the value of releasedOn is being changed from a string to an array, which cases the tests to fail. Also, a number of really early challenges don't have a release date, but the repack script adds an empty array anyway.

Took a look at the unpack, unpackedChallenge, and repack files, but not really sure where to start. Will block this for now, but hopefully we can get these all working soon.

@scissorsneedfoodtoo scissorsneedfoodtoo added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. status: needs investigation To be applied to issues if they need more in depth technical analysis prior a fix. labels Jul 8, 2018
@gforce2k5
Copy link
Contributor Author

gforce2k5 commented Jul 8, 2018

@scissorsneedfoodtoo Thank you for the feedback, I'll try to mend the issues you pointed here, and I'll try to make this script work, I'll notify you if I manage to fix these issues.

@scissorsneedfoodtoo
Copy link
Contributor

@gforce2k5, sounds great! Thank you for your continued efforts and support with QAing some of the other PRs. Please let me know if I can be of assistance in any way.

@ValeraS
Copy link
Contributor

ValeraS commented Jul 10, 2018

@gforce2k5 Do you mind if I'll contribute for the issue?

@scissorsneedfoodtoo Can you explain fields seed, ChallengeSeed, head, and tail? They are in unpacked html but not in packed json and schema. Are they needed or not?

@gforce2k5
Copy link
Contributor Author

@ValeraS I don't mind if you contribute, I managed to get the unpack script working, the only problem seems to be the repack script

@scissorsneedfoodtoo
Copy link
Contributor

@ValeraS, that's a good question. It seems like the new json structure broke some things with the unpack/repack scripts. Even then, there was some confusion over the difference between seed and challengeSeed.

It would be great if we could have the unpacked HTML mimic the new structure here: https://github.com/freeCodeCamp/curriculum#challenge-structure

With the updated scripts from this PR, it seems like only the description, releasedOn date, solution, and translations are being displayed properly in the unpacked HTML. A good example of the head field not being displayed is the curriculum/unpacked/08-coding-interview-prep/project-euler/021-problem-22-names-scores....html file.

So I would say the things we really need are the title, description, contents (formerly challengeSeed--what shows up initially in the editor), translations, tests, solution, head, tail. We don't necessarily need releasedOn since many challenges don't have a release date, but it probably wouldn't hurt. And we definitely don't need the currently existing seed and challengeSeed fields, since I think those are replaced by contents.

@ValeraS
Copy link
Contributor

ValeraS commented Jul 10, 2018

@scissorsneedfoodtoo Created new PR #127

@scissorsneedfoodtoo
Copy link
Contributor

Sorry for the delay, @gforce2k5. Thank you for your patience and for all of your other contributions in the meantime.

So I've done some testing on @ValeraS's PR and it seems good. Though since it's based on your work here, I'd like for you to also get some credit. I'm thinking we can merge your PR here, then merge #127. But maybe there's a better way to do that. What do you think, @raisedadead?

@scissorsneedfoodtoo scissorsneedfoodtoo removed the status: needs investigation To be applied to issues if they need more in depth technical analysis prior a fix. label Jul 19, 2018
@scissorsneedfoodtoo
Copy link
Contributor

@gforce2k5, sorry again for the delay. After more testing, it seems like there would be too many issues with merge conflicts if I merge this PR and then #127. Unfortunately the easiest thing seems to be to close this in favor of #127.

I'm very sorry about that. Thank you for all of your patience and hard work you put into solving this issue. Though we couldn't take your contribution this time around, we're really looking forward to your next one!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running unpack and then repack removes translations from challenge json files
4 participants