Skip to content
This repository has been archived by the owner on Dec 17, 2022. It is now read-only.

Add prompt #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add prompt #7

wants to merge 3 commits into from

Conversation

Rodrigo54
Copy link

@Rodrigo54 Rodrigo54 commented Apr 15, 2019

add interactive prompt for addon creation, as issue #1

gif-kagrenac

@haggen
Copy link
Contributor

haggen commented Apr 27, 2019

Hey @Rodrigo54!

First of all sorry for the delay, I've been swamped with work stuff these past days. Secondly, I appreciate very much the time and effort you took to contribute to this project. Thank you!

Now to the PR at hand. I have a few issues I'd like to bring to your attention to before we can proceed to merge it.

  1. What jumps to me immediately is that you made several changes but only committed once. It'd be nice to have it broke down into chunks of related/dependent changes, each one a separate commit. For instance you add a bin name and upgraded one of the dependencies in package.json that isn't really related to the prompt you were adding. These kind of small changes can have an impact later on but being mixed with other commits makes them hard to pin down.
  2. Which leads us into the second point; this merge should be about the prompt and the prompt only. If you think other changes are in order but they're unrelated to this one, please submit them in a different pull request. This makes the history of the code base way more easy to understand and possible issues become more apparent.
  3. While adding the prompt part you actually cut off a pretty big feature in my opinion; that is the ability to use arbitrary files/folder structure in the templates. Honestly I haven't run the code yet, but if I understood it correctly, from here e40ba65#diff-3ed44885788664ad9a14a40798cb75b3R14 to the next new lines, you now require specific name files for the template to work. I was kinda hoping to not have to do that. 😝
  4. Overall I like the prompt part but I would to like the PR better if you to separate the other changes, open a new pull request for them, and there expand on your motivation for them cause they are not clear to me.

Then again, it's just my feedback and I am definitely grateful for your collaboration. 👍

Oh also bonus points for including a GIF of the feature in the PR description! That's really cool!

@Rodrigo54
Copy link
Author

Thank you for responding to my pull request.

I intend to take advantage of this project to automate the delivery of my addons and when I found this project I really got carried away and I ended up committing excesses, I'm sorry. 😅

in relation to their topics

  1. I will remove everything that is not related to pull request
  2. some things did not work on the windows system which generated the need to update or replace some dependencies as was the case of exchanging platform-folders for os module
  3. in the windows system some characters are reserved and the current structure would prevent the creation of the files <%=name%>.lua and <%=name%>.txt

I believe it is important to support the windows system.

maybe we can keep the current resource to generate the files if somehow we change the template syntax instead of <%=name%> we use {{ name }} but I do not know how much that would affect a code in lua.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants