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

Add save command. #25

Closed
wants to merge 17 commits into from
Closed

Add save command. #25

wants to merge 17 commits into from

Conversation

fennecdjay
Copy link
Collaborator

Here it is ;-)
It looks just fine to me.

@fennecdjay fennecdjay added this to the 0.1.0 milestone Jan 30, 2017
@desyncr desyncr assigned desyncr and fennecdjay and unassigned desyncr Jan 31, 2017
@fennecdjay
Copy link
Collaborator Author

We're almost done!
I'd just like rmdir_r() to remove user directory if it is empty.

@desyncr
Copy link
Owner

desyncr commented Jan 31, 2017

Nice! I'm gonna take a look tonight!

Copy link
Owner

@desyncr desyncr left a comment

Choose a reason for hiding this comment

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

Some minor changes.

README.md Outdated
To remove plugin:

```sh
zpm remove user/plugin
Copy link
Owner

Choose a reason for hiding this comment

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

Not actually necessary but to keep a unified syntax on the README.md and in general, it should quote the plugin name. ie: zpm remove "user/plugin"

zpm.c Outdated
printf("#!/bin/sh\n");
memset(entry, 9, PATH_MAX);
while( fgets(entry, PATH_MAX, store)) {
printf("zpm %s", entry);
Copy link
Owner

Choose a reason for hiding this comment

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

Will need quotes on plugin name.

To make a copy of your config

```sh
zpm save > .zpmrc
Copy link
Owner

Choose a reason for hiding this comment

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

We probably will need to explain the workflow with an example. Ex:

Install some plugins:

zpm "some/plugin"
zpm "other/plugin"

Then you can persist your current configuration to be able to commit it into a dotfile repository:

zpm save > ~/dotfiles/.zpmrc

Next time you'll only need to install them like so:

source ~/dotfiles/.zpmrc

zpm.c Outdated
char* zpm_init = get_plugin_list_path();
FILE* store = fopen(zpm_init, "r");

printf("#!/bin/sh\n");
Copy link
Owner

Choose a reason for hiding this comment

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

I believe there is no need for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I just tought it was more correct.
Removing it.

@fennecdjay
Copy link
Collaborator Author

Maybe you should rework the README.md, as you clearly know zpm's workflow better than I do.

@desyncr
Copy link
Owner

desyncr commented Feb 2, 2017

Seems fair. I'll create another issue for the README.md

@desyncr
Copy link
Owner

desyncr commented Feb 2, 2017

Last thing would be to add remove and uninstall to the list of available commands.

@desyncr
Copy link
Owner

desyncr commented Feb 2, 2017

On a quick test it failed to uninstall the plugin directory. It seems it removed the .git directory though.

I also believe we could change terms: "remove" for unlinking/remove directory, and "disable" to remove plugin from listing. It's a small thing but it's best to get it correct the first time.

@fennecdjay
Copy link
Collaborator Author

If you agree, I'll comment everything in rmdir_r(), and just use something like

system("rm -r xxx")

so we get the functionnality, until I fix that function.

@desyncr
Copy link
Owner

desyncr commented Feb 2, 2017

@fennecdjay I do agree. For now it'll suffice.

@fennecdjay
Copy link
Collaborator Author

We can ignore Travis warning for now, as it complains about rake (?).
On my station it seems to work just fine.

@fennecdjay
Copy link
Collaborator Author

Also, I see you created zpm-project organisation, so I suppose you expect next PR to be there.

@fennecdjay
Copy link
Collaborator Author

Oups...
Pushing the rigth thing now.

@desyncr
Copy link
Owner

desyncr commented Feb 3, 2017

@fennecdjay There is a lot of comments and useful interactions, so let's keep it here until the final approval. Then we'll move to zpm-project. Sorry for the mess up but it has to be sooner or later :)

@fennecdjay
Copy link
Collaborator Author

Very good to me. ;-)

Copy link
Owner

@desyncr desyncr left a comment

Choose a reason for hiding this comment

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

One last thing.

zpm.c Outdated
@@ -217,6 +217,118 @@ int mkdir_p(const char *path) {
return 0;
}

int rmdir_r(const char *path) {
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Remove all this commented out code.

zpm.t Outdated
@@ -31,8 +31,8 @@ Subsequentially add a new item to the list.
List command show list of installed plugins.

$ ./zpm list
zsh-users/zsh-syntax-highlighting
rupa/z
zsh-users/zsh-syntax-highlighting .*
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably add tests for this but for now I want to get this merged ASAP to avoid huge git conflicts.

Copy link
Collaborator Author

@fennecdjay fennecdjay Feb 3, 2017

Choose a reason for hiding this comment

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

Fine. reverting to master version for now.

@desyncr
Copy link
Owner

desyncr commented Feb 3, 2017

One last thing then we can merge this. Once done you have to create a new PR in zpm-project/zpm-zsh repository.

@fennecdjay
Copy link
Collaborator Author

I'm waiting for your signal to either do more changes or create PR in zpm-project/zpm-zsh repository.

Copy link
Owner

@desyncr desyncr left a comment

Choose a reason for hiding this comment

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

We are done! Great job! Let's move it to zpm-project/zpm-zsh repository.

@fennecdjay
Copy link
Collaborator Author

Great! doing it rigth now.

@desyncr
Copy link
Owner

desyncr commented Feb 3, 2017

Also, remember to review zpm-project#6

@fennecdjay
Copy link
Collaborator Author

Sure thing!
Yet I have to figure out how to do so ;-)

@desyncr
Copy link
Owner

desyncr commented Feb 3, 2017

Moved to zpm-project#12

@desyncr desyncr closed this Feb 3, 2017
desyncr added a commit that referenced this pull request Feb 8, 2017
…nfiguration

Add OS X to travis ci configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants