Closed gillchristian closed 3 years ago
Hi @gillchristian! Thanks for taking this on.
Template names having to be part of the provided path. Maybe use different names for each command line options? (in example below)
I'm not sure what the best approach is here. All of the base
templates will apply in a repository, but the js
templates will only apply if you provide the --uses-js
flag. There are some unique files in the js
templates, and there are some files which overwrite a file of the same name from the base
templates. It's all very simplistic.
This is an issue for an approach like this:
contrib-updater generate \
--files-base README.md \
--files-base .github/workflows/ci.yml \
--files-js .github/workflows/ci.yml
...because the --files-base .github/workflows/ci.yml
will immediately be overwritten by the --files-js .github/workflows/ci.yml
. I think it would be better to just accept paths without prefixes (ie. .github/workflows/ci.yml
) and then defer to the --uses-js
flag to decide which template to use.
It's basically the same thing that already happens: if you generate the files and use --uses-js
then you are preferring the js
version of any template which also exists in the base
templates.
Use a comma separated list instead of repeating the argument? I'll check if purescript-optparse supports that.
This is probably a good idea. The library recommends implementing your own ReadM
for that case, and includes an example of doing so specifically for a comma-separated list with a default here:
https://github.com/f-o-a-m/purescript-optparse#common-builder-patterns
Validation of the given file paths. Should we error out when an invalid path is provided or just ignore like current implementation? In the example below there's an invalid file (non existing in the template) and also a conflicting file present in both base and js. Should the later case default to js when --use-js is provided?
If it isn't too difficult it would be nice to verify the path and error out if it's invalid. We already know the set of possible paths -- for example, these are the valid base
paths:
We could even use the --uses-js
flag to decide which array to search. For example, if the flag is present we first search the JS templates, then fall back to the base templates; when it is not, we can just search the base templates.
As far as defaulting when conflicting files are present, we should always defer to --uses-js
and if it is present choose the JS template.
List the generated files in the output. Is this something wanted?
Yes, this would be a nice addition!
Should the filtering be done in the runTemplates function? I did it there to do it in one place. But not just if runTemplates should be on charge of filtering 🤔
I think it is OK for this to happen in runTemplates
for now.
The current code is not very good: we just call runTemplates
on all the base templates, and then if --uses-js
is present we call it again on the js templates. This is not good because it means that the files generated in the runBaseTemplates
will get saved to the backups directery when they're overwritten by the runJsTemplates
call!
In the future it would be preferable for us to have a step where we decide what templates should be run (by checking the --files
flag, and the --uses-js
flag, and using those in combination with base
). Then we can have a single runTemplates
call instead of two.
This is where that happens:
All of that said, I don't think you need to do that in this PR -- I don't want to put too much on your plate. For now it's fine for the filter to happen in runTemplates
and we can create a followup issue if you don't want to address that here.
Thanks @thomashoneyman for pointing me in the right direction.
I refactored the way the types of the templates (base
and js
) are run. This solves the problem of js
ones overriding the same ones from base
.
TODO
EDIT: are you using any specific formatting? I run purty but it changes everything. So I tried to follow the styles of the rest of the code, maybe I missed something :sweat_smile:
UPDATE: added validation and comma separated list to the option. I'll leave the printing of generated files for another PR, if that's not a problem?
Totally fine to leave printing the files for another PR. With regards to formatting, I don’t use a formatter — there are some issues with purty that keep me from applying it across these repositories just yet. So long as you stay somewhat close to the existing style no worries on that front! From what I can tell it looks good.
@gillchristian I resolved the conflicts with main
for you as they were quite small -- if you're working locally you'll want to pull those down. Or, if you'd rather resolve yourself feel free to force push. Thanks!
@thomashoneyman no problem. After the merge I'm getting this error when running (I'm testing on purescript-nullable)
It's caused by the new changelog stuff. But I did not debug further on way it happens.
appendReleaseInfoToChangelog { owner: variables.owner, repo: spago.name }
I'm checking on that error to resolve it for you. That said, it is probably for the best that we only append release info to the changelog if it was in the included templates the user wanted -- otherwise that will fail. That complicates things a little, as now we have the changelog + a followup changelog-only action, which may not fit neatly into the code you have here; if not, we can create an issue for someone else to follow up on after your PR is merged.
@gillchristian I have a couple of suggestions that can be automatically applied, but otherwise I am ready to merge this. Thanks for your work on this and I appreciate how quickly you've been responsive to feedback! 🙇
Thanks for your work on this and I appreciate how quickly you've been responsive to feedback!
Happy to contribute! And thank you as well for the nice reviews and suggestions.
Sorry for the delay. I said "LGTM" by responding to the notification email I got. I was notified today that the email never made it.
Closes #11
This PR adds a
--files
flag to list files to generate, instead of generating the only template.Validation:
Also changes the way templates are processed avoiding running the same template twice for
base
andjs
. For common templates, when--uses-js
is provided, it will pick the JS version of the template and run only that one.NOTE: this is just the minimal implementation to see something working, which can be seen in the asciicast:
Please point let me know if something should be done differently.
Open questions and things I want to cover in the final version of the PR
base
andjs
. Should the later case default tojs
when--use-js
is provided?purescript-optparse
supports that.runTemplates
function? I did it there to do it in one place. But not just ifrunTemplates
should be on charge of filtering :thinking:~