Skip to content
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

Template as a zip #109

Merged
merged 10 commits into from
May 20, 2016
Merged

Template as a zip #109

merged 10 commits into from
May 20, 2016

Conversation

lizell
Copy link
Member

@lizell lizell commented May 18, 2016

Added support for the template to point to a local zip file (ending in .zip) of a template repository. This is useful if the template is on a private repo or not on GitHub.

As a bonus init now respects the general directory option. Before this change giving the directory option affected where roc was looking for configuration files and stuff, but created the project in the current working directory. Now the project is created at the given directory. If the option is not used it should work as before.

@@ -298,7 +309,7 @@ function askForDirectory(resolve) {
name: 'name',
message: 'What do you want to name the directory?'
}], ({ name }) => {
const directoryPath = path.join(process.cwd(), name);
const directoryPath = name.indexOf('/') === 0 ? name : path.join(process.cwd(), name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that process.cwd() should be changed to directory since we now get it from the command object?

I assume we are using indexOf here to check if this is a absolutePath so we can create the directory outside the current scope if we want. Maybe we should rewrite this to use the getAbsolutePath utility function that we have https://github.com/rocjs/roc/blob/master/src/helpers/index.js#L13 that would both make the code more robust and easier to parse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's is why I had the directory passed along to askForDirectory first. I removed and figured why did I ever want this here! ;) Thx. I'll fix that and also use getAbsolutePath!

@dlmr dlmr added the feature label May 18, 2016
return new Promise((resolve) => {
const directoryPath = path.join(process.cwd(), directoryName);
const directoryPath = getAbsolutePath(directory || process.cwd());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to change the signature for checkFolder to (force, directoryName, directory) and use directory as the second argument to getAbsolutePath. That means that https://github.com/rocjs/roc/pull/109/files#diff-03189079b1b3d77ef73e9e3fdb324d6dR47 would be similar to before.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, but I don't think it will work. In order to keep the behavior in my added test cases I would have to

getAbsolutePath(directoryName || directory || process.cwd())

as neither directoryName or directory are mandatory (getAbsolutePath returns undefined if the filepath is not defined.)

This is the way I think it should work:

# directoryName directory directoryPath
1 undefined undefined /cwd
2 name undefined /cwd/name
3 /name undefined /name
4 undefined directory /cwd/directory
5 undefined /directory /directory
6 name directory /cwd/directory/name
7 name /directory /directory/name

6 and 7 will not work, so to support that we could do something like:

getAbsolutePath(path.join(directory, directoryName));

Do you think it would work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

I think the right thing here might be to update getAbsolutePath so that it does not return undefined when filepath is undefined but instead either process.cwd() or the directory that was provided to it. Seems like this is the expected result in the code but I will need to look a little closer to be 100% sure if we decide to go down this path.

When running init the directory will always be absolute when running in Roc. This means the cases 4 and 6 will never happen in the table above. Or rather the path will already be /cwd/directory when processing init from the cli. This means that some tests can be updated to reflect this.

I did these changes locally and the tests seems pass correctly, or all but one. roc commands init should choose name over directory does fail but I believe that it tests an incorrect/unexpected behaviour. For me the expected behaviour is the following:

roc new name --directory directory // Should try init inside /cwd/directory/name 

The most important change this would bring is that all mentions of process.cwd() in the Roc commands functions should be replaced with the new directory value that by default will be process.cwd() but can be configured by the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yepp I had updated my test, but didn't push it, since I wanted your view on things. About case 4 and 6, isn't your example case 6 exactly?

Maybe I should push the latest code including the getAbsolutePath(path.join(directory, directoryName)); and we do the process.cwd() replacing in a separate branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, let's do that! 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pushed. Should I create an issue for the refactoring of getAbsolutePath or are you already on it?

Copy link
Member

@dlmr dlmr May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda on it in my next branch where I make the needed changes for bringing us to a stable 1.0. And I think we will need to wait with this change for that since I now have looked more closely into this and there is extensions that expects to get undefined in some places. But feel free to create a issue to make sure this is tracked correctly!

I will go ahead and merge this if the build passes! 👍

@dlmr
Copy link
Member

dlmr commented May 19, 2016

I have tested this out now and it works awesomely! 👍

I'm almost ready to merge.

Just the two questions above and for some reason it seems that the tests are outputting things to the console that was not present before in regards to the init tests. Could you take a quick look at it and see if it is something that can be easily addressed?

@lizell
Copy link
Member Author

lizell commented May 19, 2016

Ooops, I missed to wrap my added tests in the consoleMockWrapper. I will fix that!

@dlmr dlmr merged commit 28189be into master May 20, 2016
@dlmr dlmr deleted the template-as-a-zip branch July 1, 2016 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants