-
Notifications
You must be signed in to change notification settings - Fork 300
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
Unit testing #102
base: main
Are you sure you want to change the base?
Unit testing #102
Conversation
Thanks very much for the MR! I will try to review it soon (weekend or next week). First look is that is good, just need time to read it properly :-) |
@cpina You seem very busy, and I don't see a lot of movement in the repository would you be interested on adding me as a mantainer? |
This PR felt into other things that I've been doing. I will have a better look soon. Thanks again for the PR, I should have acted (merge, feedback, etc.) sooner. When I had a quick look in May I thought that I was in a catch-22 situation: I don't want to change the code without tests, but to implement the tests, in the PR, the code is changing :-) (and I really want to avoid regressions). I've had a look now and it's not as different as I had initially thought. Regarding maintainer: I'd like to make the repo more sustainable, so I'll think about it (I have some concerns either way). Let's see first where I get with the PR :-) Thanks for offer your help though (and for the good PR with the needed tests and re-factoring). |
No worries (I always get ghosted on PRs), and forgetting to do things is basically my thing so it's truly alright.
I have purposefully made it as similar as the original as I could just splitting it into functions so it could be tested, further more if you don't like the changes that I propose (that are truly little over at a2e083e ) you can just throw away that commit.
It's good to have concerns (trust, communication, etc), but being a lone maintainer is heavy, so even if it's not me (because you don't know me or trust me) you should look into having another maintainer. I hope I can hear your feedback soon. |
@cpina RFC |
My thoughts at the moment:
Side topic: My plans for when I get into this: I want to add an explanation (in the FAQ) in the docs on how to use .gitignore (as mentioned in some issue or MR) to avoid copying / overwriting files. I thought that it was a clever way, and .gitignore has many, many features on includes and excludes. Adding options to the action for this would be endless (exclude a directory, include subdirectories, exclude files..), if it can use .gitignore we can cover many things (which I can if I remember correctly). I also want to add a list of related github-actions (other actions that serve the same or similar purpose). Not all of them can do everything, and it would be useful to have a bit of a list for when one of them does not fit a use case. @pwall2222 : an idea is that you fork this one, merge your MR and I add yours (and others!) in the docs. If you do: ping me here and I'll add this section in the docs. I guess that this is probably not your desired way (else you would have already done) but I think that's a way to move forward. Perhaps, when I have time to write integration tests to ensure that nothing gets broken then I even merge it back? Who knows! For now I'm happy to leave the MR open because I still find it interesting! Feel free to get in touch for any comments or thoughts! |
It's a huge paradigm shift from the one script to rule them all, but the only way to do unit tests is if we have units and functions seem the smallest units of shell scripting appropriate to test.