zpm-project / zpm-zsh

zsh plugin manager in ansi C.
GNU General Public License v3.0
5 stars 3 forks source link

Rework the plugin_entry function #11

Open desyncr opened 7 years ago

desyncr commented 7 years ago

See https://github.com/zpm-project/zpm-zsh/issues/9#issuecomment-277246465

fennecdjay commented 7 years ago

Yes.

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

desyncr commented 7 years ago

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

 Plugin has no sourceables. Ignoring.

Or something better :)

fennecdjay commented 7 years ago

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

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 commented 7 years ago

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

desyncr commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

should this be recursive?

desyncr commented 7 years ago

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 commented 7 years ago

What do you think?

Quite the same 😄 .

fennecdjay commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

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

My implementation is faulty.