Imscp preset

[rtibbles]

• Updates the generation script to call pre-commit internally on generated files
• Adds a pre-commit hook to regenerate files if any of the specs have changed or if the setup.py has been modified (as a way of regenerating for a new version)
• Adds a format preset for the IMS Content Packages - so that we can upload them but not have them be importable by older Kolibris
• Bumps the version

N.B. This will not stop all Kolibris from importing them, as very old Kolibris rely on the content kind, and to avoid a kind proliferation, I have kept the kind here as HTML5. However, that is only on versions older than 0.11.x, of which there are very few.

[nucleogenesis]

Just one comment about the changes to pre-commit - I'm sure it isn't but it looks like pre-commit could end up circular if pre-commit now runs make build which calls the generate_from_specs which then calls pre-commit again on a particular set of files that it has changed.

[nucleogenesis]

The diff is straightforward here and love calling pre-commit after generation

[nucleogenesis]

Does this run make build whenever we run pre-commit run?

[rtibbles]

Yes, but only if any of the spec JSON files or setup.py have been modified (as per the regex below).

[bjester]

Just one comment about the changes to pre-commit - I'm sure it isn't but it looks like pre-commit could end up circular if pre-commit now runs make build which calls the generate_from_specs which then calls pre-commit again on a particular set of files that it has changed.

I saw this too. Dropping in my thoughts

[bjester]

I'm not a huge fan of the recursion through pre-commit that I think is happening with these changes.

So to make sure I understand, you added to the pre-commit config to run make build when the source files run, and thereby removed this part of make build where it runs pre-commit, which I recall was added to reformat the generated files in a consistent manner. But then in scripts/generate_from_specs.py, you added a call to run pre-commit but only on the output files:

subprocess.call(["pre-commit", "run", "--files"] + output_files)

So technically the new pre-commit hook shouldn't run against the output files because they shouldn't match the regex, thereby not triggering make build again, but still the new hook would take pre-commit through make, python, then back to pre-commit again. I think I had brought up potentially having python scripts/generate_from_specs.py output the list of files for pre-commit.

[rtibbles]

To respond to both of you - yes, I had deliberately scoped the pre-commit invocation from within the generation script to only the generated files, so as to avoid the recursion.

I originally did not do it this way, and instead thought that just by putting the generation hook first, that would somehow be OK, but pre-commit doesn't rescan for newly changed files during the pre-commit process, so even as the first hook, the new files did not get picked up by subsequent hook executions and so were not reformatted.

Then if you go to recommit those newly generated files to let them get reformatted, the original change that triggered the new hook in the first place retriggers the new hook and I ended up in a loop of trying to commit and failing because the files were getting formatted and unformatted in quick succession.

This didn't suffer from any of those problems, and because the outputted files always conform precisely to the requirements of pre-commit because they are all run through it in the generation script, there are never any issues with the subsequent hook invocations too.

[bjester]

I do think it would be better to have make build be a manual process, outside of pre-commit. We would always have the PR check to ensure files are regenerated, since pre-commit could be missing or skipped. So it feels we're doubling up the automation and breaking a boundary which perhaps exists for a good reason-- you should be able to see the full diff of what you're committing.

[rtibbles]

you should be able to see the full diff of what you're committing.

That's still the case - nothing is committed outside of user control here, it just ensures that when you commit changes that should result in changes in generated files, those changes are generated and now are available to be committed.

[rtibbles]

We also don't currently have a check to make sure generated files are regenerated when needed - the only check is to make sure that no finalized specs are modified. So this adds this missing check.

[bjester]

Oh I see. It does still fail the first commit. I thought that wouldn't be the case. Sorry for the confusion.

I still dislike this change though. First, I tried it and the double output will surely confuse someone. Secondly, within pre-commit, I think applying code formatting is acceptable because the code shouldn't functionally change. With the scripts/generate_from_specs.py, it's generating actual code which will be imported broadly into Kolibri's and browsers through the published packages. So having that happen automatically in pre-commit, where it's too easy to just accept the auto changes from it, makes me uneasy about the placement of this automation within pre-commit specifically.

[rtibbles]

OK - to avoid stalling this unnecessarily, I've backed out the change to pre-commit but left everything else the same.

We can revisit how to add an automated check that files have been rebuilt in the future.