-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add kindergarten-garden exercise #248
Add kindergarten-garden exercise #248
Conversation
Hello 👋 Thanks for your PR. This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed. If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track. Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum. (cc @exercism/cross-track-maintainers) |
*.o | ||
tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not necessary, as we have these ignored in the project root. I would remove this .gitignore
file unless or until there is something "custom" specifically needed for an exercise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed they are for the benefit of students who use git to manage their solution attempts on their local machines.
exercism/rust#353
mentions "I like to keep my Exercism solutions backed up on GitHub (e.g. https://github.com/attilahorvath/exercism-rust) and I've seen others do the same."
The following tracks currently use per-exercise .gitignore:
- elixir
- gleam
- javascript
- purescript
- rust
- swift
- x86-64-assembly
I don't have a preference either way. They were there when I started adding x86-64-assembly exercises, and I followed the existing convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing here is a critique against your response. Thank you for that. Here is my reasoning...
The .gitignore
files are getting delivered for the exercises, which have have nothing to do with git. Sounds like "personal choice" to me, and additional information that is not really relevant to the exercises.
A slippery slope, for that reason, as "I assume they are for the benefit of students using X editor" would also justify adding editor related things not only to that .gitignore
file, but also delivering it with those things in there.
I would argue that us delivering it per exercise causes more student maintenance work, as well, and they should manage their own repositories if that is what they are doing.
I am not against having a documentation thing that describes how they can do such things, and recommended things to ignore though. But that is a one place area to maintain, and specifically for the track, rather than per exercise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I have removed the .gitignore file from the current PR. Note I also have a handful of other exercise-adding PRs uploaded for review. After they get reviewed/merged, I'll delete all of the track's per-exercise .gitignore files.
generators/generate
Outdated
for i in data["cases"]: | ||
if "cases" in i: | ||
for j in i["cases"]: | ||
add_case(cases_by_id, j) | ||
|
||
def traverse(node): | ||
nonlocal cases_by_id | ||
if "cases" in node: | ||
for child in node["cases"]: | ||
traverse(child) | ||
else: | ||
add_case(cases_by_id, i) | ||
add_case(cases_by_id, node) | ||
|
||
traverse(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This set of changes seems to be unrelated to the purpose of the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generator previously did not allow for deeply nested tests.
If I omit this change, we have
Traceback (most recent call last):
File "generators/generate", line 111, in <module>
main()
File "generators/generate", line 96, in main
cases = flatten_cases(data)
^^^^^^^^^^^^^^^^^^^
File "generators/generate", line 34, in flatten_cases
add_case(cases_by_id, j)
File "generators/generate", line 44, in add_case
cases_by_id[case["uuid"]] = case
~~~~^^^^^^^^
KeyError: 'uuid'
If I just made a one-line generator change
def add_case(cases_by_id, case):
if "reimplements" in case:
cases_by_id[case["reimplements"]] = case
- else:
+ elif "uuid" in case:
cases_by_id[case["uuid"]] = case
then generation reports no errors, but omits two test cases:
- RUN_TEST(test_second_students_garden);
- RUN_TEST(test_third_students_garden);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see a separate commit for this explaining that.
Do we need maybe reform the patch and force push on this feature branch to update and separate in case one thing needs to be reverted but not the other things that may be good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted #249 that can be merged first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the #249 are we going to modify this patch to exclude it? It still should not conflict, if the other is done first, but as it is it seems "off" just a bit.
I do not have enough rights on this repository to fix it up, otherwise I would do that, instead of tossing the bone back your way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now modified.
I appreciate the difficult work that you and the cross-track-maintainers-team are doing. Would it be helpful if the first commit in any new exercise was the result of a |
78c86b9
to
b0cb909
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, it looks good.
Uses recently updated generator to allow for deep nesting of tests in canonical json