Skip to content

Conversation

@fennecdjay
Copy link
Member

@fennecdjay fennecdjay commented Feb 16, 2017

It should benefit from something about in the README, like

# .zshrc
export ZPM_COLORS=1 # enable zpm color output

@fennecdjay
Copy link
Member Author

I think we should merge it as soon as reviewed, and all all log and debug lines later, when they are clearly defined on the board.

@fennecdjay
Copy link
Member Author

Could you grant me some rigths on this repo, so that I can handle issues, labels, request reviews...
Also I don't need acces on [this one](https://github.com/desyncr/zpm-zsh anymore) 😄

@fennecdjay
Copy link
Member Author

@desyncr wrote:

Yep a --color flag would also be useful.

I think just having zpm check the environment is fine.
Plus a --color flags would make argument parsing a bit longer 😄

@desyncr
Copy link
Member

desyncr commented Feb 16, 2017

You should have those rights already as you're a org member. I'm gonna see what's going on later.

@desyncr
Copy link
Member

desyncr commented Feb 16, 2017

Current argument parsing is a bit rudimentary so we could look into it.

@fennecdjay
Copy link
Member Author

Current argument parsing is a bit rudimentary so we could look into it.

I quit like it like that 😄
maybe we should just warn when there are extra-arguments on the command line.

Copy link
Member

@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.

Two small things to review.

free(hash);
} else {
printf("\n");
if(zpm_has_color) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the need to for this condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • because bold relates to fancy output.
  • because bold messes with cram 😕

vsnprintf(msg, 256, fmt, arg);
if (type != ZPM_NONE) {
if(zpm_has_color) {
int color = type == ZPM_ERR ? 31 : 32;
Copy link
Member

Choose a reason for hiding this comment

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

Check coding style.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I mostly pasted untested code from the issue, so while fixing I forgot that 😕

@desyncr
Copy link
Member

desyncr commented Feb 16, 2017

Well, we should be able to extend zpm and add new commands, options, flags without too much fuzz; currently it's quite messy.

I think for now it's OK to use env vars, I'll keep on the backlog to investigate option parting libs.

@desyncr
Copy link
Member

desyncr commented Feb 16, 2017

@fennecdjay Re: permissions, sorry my bad. I miss configured permissions for members. Now you should have write access. 👍

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.

2 participants