Create new track for GDScript

[codingthat]

@Meatball

About the jq dependency (for students not running Docker), I see now that the JSON requirement comes from exercism itself. But, handily, GDScript has JSON support built-in. So if we really want, we can ditch jq in favor of a GDScript script. Do you both agree?

Yeah if Json could be handeld through Gdscript would that be preferable. Alternativly Powershell can do JSON parsing, so an option if that doesn’t work would be to use: bash with jq and Powershell. And there is jq for Windows as well.

Super, I will work on the JSON-in-GDScript front.

[IsaacG]

You may want to run bash scripts through shellcheck to ensure best practices are followed.

[codingthat]

Good point. Will do.

[codingthat]

@codingthat

@Meatball

About the jq dependency (for students not running Docker), I see now that the JSON requirement comes from exercism itself. But, handily, GDScript has JSON support built-in. So if we really want, we can ditch jq in favor of a GDScript script. Do you both agree?

Yeah if Json could be handeld through Gdscript would that be preferable. Alternativly Powershell can do JSON parsing, so an option if that doesn’t work would be to use: bash with jq and Powershell. And there is jq for Windows as well.

Super, I will work on the JSON-in-GDScript front.

So…upon closer inspection, this seems to already be implemented, actually. At some point the jq dependency in the Docker setup was removed, yet stayed on its installation list. I think we can remove all trace of jq since we already have what we need inside the GDScript end of the test runner.

The easiest solution I can see would be:

• Call godot --headless directly from the local test runner script instead of via run.sh, since run.sh’s exports aren’t needed outside Docker. (I.e., on my system, replacing the local test runner’s run.sh line with a direct godot --headless -s bin/test_runner.gd 2>/tmp/stderr -- works fine.)
• Let the GDScript test runner, which currently requires 3 params, have the 3rd (the JSON output directory) be optional; if not supplied, it can, instead of writing JSON to a file, output test results to stdout (as JSON or something even prettier).
• To this end, rename file_utils.write_results_file to a more generic output_results and have it likewise have an optional 3rd param.

@Meatball are you OK with this approach?

(Either way, I’d then run shellcheck on the final bash script.)

[codingthat]

@codingthat

At some point the jq dependency in the Docker setup was removed, yet stayed on its installation list.

@Meatball I PR’d this removal if you want to run any tests before merging, but the CI uses the Dockerfile directly, so given that it didn’t break, I think we’re OK: Remove jq from Dockerfile since its only use was removed by codingthat · Pull Request #53 · exercism/gdscript-test-runner · GitHub

[Meatball]

Sounds good

[codingthat]

Turns out the jq removal broke the main gdscript repo* because it depends on the test runner repo, which makes sense. It’s unfortunate design though that CI checks on the test runner repo won’t ever catch that dependency.

* In the sense that the next time the main repo’s CI ran, even though it was only a change to a Markdown file, the CI failed.

Rather than revert, though, I’d like to implement the above strategy to remove the need for jq — it just means a bit more work on the main gdscript repo as well. Let’s see what I can get done. Or if this is blocking anyone for the moment, of course feel free to revert and we can redo the change once all jq dependencies are actually gone.

[codingthat]

OK @Meatball @IsaacG I’ve done the above:

Comparing exercism:main...codingthat:remove-jq-dependency · exercism/gdscript

Comparing main...remove-jq-dep-from-local-test-runner ·...

These are intended to be merged around the same time. This would:

• Fix the broken CI
• Remove all traces of jq from both repos
• Provide a friendly, colour-coded-output local test runner option instead of JSON, while maintaining compatibility with the JSON test runner spec for learners without a local setup
• Work with both Docker and local setups for testing the whole track

Thanks @IsaacG for recommending ShellCheck, it was really handy and provided fixes to preexisting code as well!

Looking forward to your feedback. Cheers.

[IsaacG]

It’s hard to comment on branch diffs. If there’s a PR, people can comment on it directly.

[codingthat]

Good call:

Remove jq dependency

Remove jq dep from local test runner

[Meatball]

Nothing against not using jq, it looks good but one of the ci fails as you have noted yourself so that needs to be fixed then I think we can merge these.

[codingthat]

Thanks, but what it was was that removing jq worked fine on the test runner repo (CI passed there) but it broke the CI on the main repo. We just didn’t notice because there wasn’t a main-repo PR for a while after that.

These PRs are to remove the jq dependency everywhere (thus fixing the CI) while adding support for the local test runner for learners to use.

[kotp]

Well, not necessarily hard, since it is “Just Git” https://github.com/exercism/gdscript/compare/main...codingthat:gdscript:remove-jq-dependency.patch

Just probably presented not as well as it could have been done.

[IsaacG]

Yeah. It’s not hard to view the diff. It’s hard to “leave a comment” when you’re used to using the nice UI tooling that GitHub provides on a PR. It’s hard in the sense that the “regular” tooling is absent.

[kotp]

When you state it in terms of “github provided tooling” that is absolutely the case. If this server was accepting e-mail, then I would argue that it is “just as easy or easier” to comment. (The patches are mbox format after all, so easy to (R)eply to in my mail client.)

[codingthat]

@Meatball

that needs to be fixed then I think we can merge these.

BTW not sure if you saw over on the PR, but we were just waiting to see if you’re OK with me converting verify-exercises (from the main repo, not the test runner) from bash to GDScript as part of the “remove jq dependency” effort. Not sure if it needed to be bash for some reason or not. Thanks in advance

[Meatball]

Yeah, that is fine, use what you find the most optimal solution.

[codingthat]

OK, great, it’s updated. gdscript-test-runner is passing and gdscript is failing, pending the former’s PR being merged, after which they should both pass (maybe with a dummy commit on the latter to re-trigger the CI).

[codingthat]

@Meatball @IsaacG , Remove jq dep from local test runner by codingthat · Pull Request #54 · exercism/gdscript-test-runner · GitHub is merged.

Upon rerunning the CI for Remove jq dependency by codingthat · Pull Request #102 · exercism/gdscript · GitHub, it now passes, too.

OK to merge the latter #102?

[codingthat]

OK, CI’s back on track (thanks to keiravillekode for some help during a moment of confusion this morning, too — and also for getting the CI updated to a newer Ubuntu version).

I think if we get the documentation finalized, then one last check that everything is up-to-date (version of Godot and syncing all exercise data) and we should be good to launch, then we can focus on adding exercises.

I’ve reviewed an older PR in more depth — [Docs]: Add installation by meatball133 · Pull Request #5 · exercism/gdscript · GitHub — and compared with Add track installation instructions by codingthat · Pull Request #97 · exercism/gdscript · GitHub :

For PR #5, I think I ended up overlapping it quite a bit. PR #97 leaves more of the installation instructions offloaded to the official Godot site, per the comment in the template, “usually it is better to link to a resource with the official installation instructions, to prevent the instructions from becoming outdated.” But it does say “usually” so I’m not stuck on this, if there’s a good reason to include further detail.

The only other bit is the Windows-specific instructions but since we don’t have PowerShell scripts yet, I think it would make sense to wait until we do to add that part. (I’m not sure what the learner benefit is to having godot runnable from PS without a test runner.)

(Meanwhile, thanks for your comments @BNAndras , I’ve updated PR #97 accordingly.)