gen: Parse version out of JSON file; leap: Show version in generated file #606
Conversation
exercises/leap/cases_test.go
Outdated
| @@ -1,7 +1,7 @@ | |||
| package leap | |||
|
|
|||
| // Source: exercism/x-common | |||
| // Commit: be6fa53 leap: Rewrite the test cases and their descriptions | |||
| // x-common version: 1.0.0 | |||
There was a problem hiding this comment.
Well, there are advantages and disadvantages.
Advantage of commit: We'll know if there's any change, even if someone forgot to change the version when test contents change (I think x-common reviewers have been good at reminding people to change version though)
Advantage of version: We know how important the change is, if there is one - for example in a patch-level change (the last number) it isn't strictly necessary to rerun the generator since no test content will change.
So let me know whether you think it's better to only see version, only see commit, or see both.
There was a problem hiding this comment.
Nice, I like having the version, I agree with the advantages of it, but I like it as an addition rather than a replacement, I think the commit pinpoints it to a specific point in time, and includes a brief but useful snippet of info regarding any recent changes.
Though if I had to choose, I'd go for the Version.
There was a problem hiding this comment.
An additional thought: I'm just starting a PR to try pulling the canonical-data.json down from Github instead of/prior to doing what it does (I just keep forgetting to update my copy of the x-common repo before regenerating! 🤷♂️ ) and as far as I can see this would mean not being able to include the commit message in the generated file. And if the generator does take the route of being able to get the current json data via http as well as from an offline repo I assume we wouldn't want to sometimes have the commit message, sometimes not?
There was a problem hiding this comment.
pulling the canonical-data.json down from Github
The only track I know of that tried to do this is exercism/crystal#57, guess that hasn't gotten completed though. I link them for reference to see if you can get any ideas from them, but feel free to implement it without looking at what they did if none of what they did is applicable to Go.
this would mean not being able to include the commit message in the generated file
How about using https://api.github.com/repos/exercism/x-common/commits?path=exercises/acronym/canonical-data.json ?
I assume we wouldn't want to sometimes have the commit message, sometimes not
Yeah seems confusing. I would like to always have it.
But since it seems possible to always have it, looks like it's possible to keep it, yeah?
|
Based on our discussion (everything in #606 (comment) is still relevant, so expand that outdated diff) I am going to show both commit and version... and that honestly makes me think that there should just be a single function that shows this header. |
I'm going to want to add a function to it.
I keep forgetting what Ori means. I know Go likes to keep short variable names, but there's got to be a balance we can strike, and especially since I keep thinking of the "Or Immediate" assembly instruction
I keep forgetting what Ori means. I know Go likes to keep short variable names, but there's got to be a balance we can strike, and especially since I keep thinking of the "Or Immediate" assembly instruction
As part of the work in #604
petertseng
dismissed
robphoenix’s stale review
The code has changed signficantly since the last approval, so I would request another review before it is safe to merge it.
| // Source: {{.Ori}} | ||
| {{if .Commit}}// Commit: {{.Commit}} | ||
| {{end}} | ||
| {{.Header}} |
There was a problem hiding this comment.
One thing I was talking about in #605 (comment) (and I should have talked about it in #604 as well):
If, in the course of writing a generator, we find that there is significant duplicate code between generators that could be extracted into https://github.com/exercism/xgo/blob/master/gen/gen.go, we should do so, so that writing each generator is as little work as possible.
This is one thing I was talking about - we keep writing this header and it's the same three lines every time (and it we add the version, it's going to be four lines). Since it's so common, I propose it be just one line.
|
Is the deprecation of Ori for Origin noted anywhere? That could be part of #604 |
No description provided.