update gen.Gen function#447
Conversation
fixes exercism#357 Since the re-organised exercises directory (exercism#224) the test generator filepaths have been broken. This commit changes `gen.Gen` to take care of locating the `canonical-data.json` file, allowing each exercises `example_gen.go` file to just pass in the exercise name. This should also help with any future changes that affect the filepaths.
this change ensures the commit mentioned in the generated cases_test.go files is relevant to the exercise. closes exercism#445
| if dirMetadata == "" { | ||
| return errors.New("unable to determine current path") | ||
| } | ||
| jFile := filepath.Join("exercises", exercise, "canonical-data.json") |
There was a problem hiding this comment.
I'm not sure how idiomatic this is. Is it worth moving the "exercises" and "canonical-data.json" strings into variables within the function or even package level so they're further up the file, making them more obvious and potentially easier to change in the future?? or not??
There was a problem hiding this comment.
For me personally, I would 100% do it if either of the following were true:
- they were magic numbers; numbers without explanations/names seem much harder to understand
- A line containing "exercises" and "canonical-data.json" appeared more than once, since I don't like repetition.
I feel less stringent about these... I guess you call these magic strings? I would definitely not oppose creating a jsonPath(exercise string) string function (you can name that whatever you want, I'm not insistent on that name), but I also don't mind having it inlined.
Also part of my decision is that it doesn't seem very likely to change, and it is also not very hard to understand what it is doing by reading it.
There was a problem hiding this comment.
ok, super, I'll leave it as is, as you say it's not hard to understand nor is it repeated. I do like the idea of a jsonPath function but it also feels like maybe one layer of abstraction too many.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
All seems reasonable. I definitely feel like #357 and #445 should get mentioned. Haven't run it myself to test, but I feel like this should just work.
The change to make all generators pass in just the exercuse slug rather than the filename would want to either reference this PR... or just be added as another commit in this PR.
| if dirMetadata == "" { | ||
| return errors.New("unable to determine current path") | ||
| } | ||
| jFile := filepath.Join("exercises", exercise, "canonical-data.json") |
There was a problem hiding this comment.
For me personally, I would 100% do it if either of the following were true:
- they were magic numbers; numbers without explanations/names seem much harder to understand
- A line containing "exercises" and "canonical-data.json" appeared more than once, since I don't like repetition.
I feel less stringent about these... I guess you call these magic strings? I would definitely not oppose creating a jsonPath(exercise string) string function (you can name that whatever you want, I'm not insistent on that name), but I also don't mind having it inlined.
Also part of my decision is that it doesn't seem very likely to change, and it is also not very hard to understand what it is doing by reading it.
Oops, that's what I get for not expanding the commit messages. I'm usually better about this, I have no excuse (I'm in a bit of a hurry, but no excuse). OK, given that you've tested it go ahead and merge. I may not get to review your other PRs for a few hours, depending. We'll see. |
|
Oh yeah, those commit messages do a good job of explaining why the change is made. I definitely shouldn't have missed them! |
|
hey, no worries, don't feel obliged at all, I'm going to just update them and then work on other things, so whenever you get a chance, and no excuse needed, c'est la vie y'know. 😄 |
reference: exercism#447 README details on generating tests updated to reflect moving exercises into exercises subdirectory
reference: exercism#447 README details on generating tests updated to reflect moving exercises into exercises subdirectory
2 participants
No description provided.