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

Rework the plugin_entry function #11

Open
desyncr opened this issue Feb 3, 2017 · 12 comments
Open

Rework the plugin_entry function #11

desyncr opened this issue Feb 3, 2017 · 12 comments
Assignees
Milestone

Comments

@desyncr
Copy link
Member

desyncr commented Feb 3, 2017

  • append to fpath if there is a completion
  • append to path if there are executables.
  • source if there is a *.*sh file.

See #9 (comment)

@desyncr desyncr added this to the v1.0.0 milestone Feb 3, 2017
@fennecdjay
Copy link
Member

Yes.

and also do not add entry if there is none of those.

@desyncr
Copy link
Member Author

desyncr commented Feb 3, 2017

In that case (which is rare), we should display a warning message:

 Plugin has no sourceables. Ignoring.

Or something better :)

@fennecdjay
Copy link
Member

Correct.
Anyway we probaly need some work on messages for v1.0.0:

  • error message should output to stderr.
  • normal message should output to stdout.

we should have zpm_message(char* message, int error) or zpm_message(int error, char* format, ...) for that.

Furthermore, it migth help fancying the output.

@desyncr
Copy link
Member Author

desyncr commented Feb 3, 2017

Yes, the output correctly if pretty raw. Any proposals regarding fancying out?

@desyncr
Copy link
Member Author

desyncr commented Feb 3, 2017

By the way, I fully agree with adding a zpm_message function, and properly handling stderr/stdout. I'd wait until we better organize code, which will be a huge refactor.

@fennecdjay
Copy link
Member

fennecdjay commented Feb 3, 2017

Probably [ZPM] to start every line except for list.
maybe green for standard message and red for error.
also plugin name should be bold.

@fennecdjay
Copy link
Member

In my opinion, the implementation for this would be to have get_plugin_entry_point() loop through all files in the plugin directory, add what is needed to ~.zsh-init.zsh and increment the return value, so that we could check if it is zero, and either

  • add the plugin to ~/.zpm/.plugin_list
  • display an error message and return 1.

should this be recursive?

@desyncr
Copy link
Member Author

desyncr commented Feb 3, 2017

I was thinking having two functions: get_plugin_has_executables and get_plugin_has_autoload (for fpath). These functions should return the number of such entries found.

get_plugin_has_executables should check for any file with executable flag set.

get_plugin_has_autoload should check if there is any autoloadable file (ie, _completion).

Both functions should be used to add conditions around these lines: https://github.com/zpm-project/zpm-zsh/blob/master/zpm.c#L105

What do you think?

Edit: Corrected description what these functions should return.

@fennecdjay
Copy link
Member

What do you think?

Quite the same 😄 .

@fennecdjay
Copy link
Member

The current get_plugin_entry_point() returns the last file matching file it finds.
Is this the intented behavior?

Shouldn't we add an entry for every match we find?

I think get_plugin_entry_point() has to be merged in get_plugin_entry(), with two more variables, has_executables and has_autoload, which would trigger PATH=xxx and fpath+=xxx writing.

@desyncr
Copy link
Member Author

desyncr commented Feb 4, 2017

I believe it should return the first occurrence. At least that was my intention.

Most plugins have a single-file entry point so I think we cover 99% of plugins by just sourcing in that order (also the logic is shared with most zsh plugin managers).

I believe generate_plugin_entry could be refactor and move fpath and PATH handling into get_plugin_entry. I'd still have two separate functions for fpath and PATH checks.

@desyncr desyncr self-assigned this Feb 7, 2017
@desyncr desyncr modified the milestones: v0.2.0, v1.0.0 Feb 16, 2017
@desyncr
Copy link
Member Author

desyncr commented Mar 9, 2017

I believe it should return the first occurrence. At least that was my intention.

^^ This is wrong. See #37 and https://github.com/zsh-users/antigen#notes-on-writing-plugins.

My implementation is faulty.

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

No branches or pull requests

2 participants