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

functions --hooks #4694

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@hamon-e
Copy link
Contributor

hamon-e commented Jan 31, 2018

Add a little options to list all events hooks
The current output it's not really pretty but i had no idea to make it better

It's my first contribution don't hesitate to criticize

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 31, 2018

This looks nice! Thanks for doing this!

I don' t know about the name "hooks," we don't use that term anywhere else. Users cause a function to respond to an event via --on-event, how do you feel about "event handlers?"

A few nits I'll put in code review. Also if you get to it you can add this to the CHANGELOG, if not I'll do it.

@fish-shell fish-shell deleted a comment from ridiculousfish Jan 31, 2018

static const struct woption long_options[] = {
{L"erase", no_argument, NULL, 'e'}, {L"description", required_argument, NULL, 'd'},
{L"names", no_argument, NULL, 'n'}, {L"all", no_argument, NULL, 'a'},
{L"help", no_argument, NULL, 'h'}, {L"query", no_argument, NULL, 'q'},
{L"copy", no_argument, NULL, 'c'}, {L"details", no_argument, NULL, 'D'},
{L"verbose", no_argument, NULL, 'v'}, {NULL, 0, NULL, 0}};
{L"verbose", no_argument, NULL, 'v'}, {L"hooks", optional_argument, NULL, 'o'},

This comment has been minimized.

@jchilders

jchilders Feb 1, 2018

Seconding @ridiculousfish's comment: "event_handlers" would be more consistent.

@hamon-e

This comment has been minimized.

Copy link
Contributor

hamon-e commented Feb 1, 2018

yeah i prefer it too ; what do you think of it ?

@hamon-e

This comment has been minimized.

Copy link
Contributor

hamon-e commented Feb 2, 2018

I don't find optional argument really pratical
so i create another one

void event_print(io_streams_t &streams, const wcstring *filter) {
std::vector<shared_ptr<event_t>> tmp;

static std::map<int, wcstring> dico = {

This comment has been minimized.

@ridiculousfish

ridiculousfish Feb 2, 2018

Member

Please just use a switch statement here (an external function or a lambda):

static const wchar_t *name_for_event_type(int which) {
   switch (which) {
     case EVENT_ANY: return L"any";
     ...
   }
}

This comment has been minimized.

@hamon-e

hamon-e Feb 8, 2018

Contributor

I also need to do the reverse operation (have a string and get a int)
and for that i can't do a switch statement

tmp = s_event_handlers;
std::sort(tmp.begin(), tmp.end(),
[](const shared_ptr<event_t> &e1, const shared_ptr<event_t> &e2) {
if (e1.get()->type == e2.get()->type) {

This comment has been minimized.

@ridiculousfish

ridiculousfish Feb 2, 2018

Member

FYI you can write e1->type == e2->type here, no need for get()

});

int type = -1;
for (std::vector<shared_ptr<event_t>>::iterator iter = tmp.begin();

This comment has been minimized.

@ridiculousfish

ridiculousfish Feb 2, 2018

Member

for (const shared_ptr<event_t> & evt : tmp) {... is nicer. fish has graduated into C++11 :)

static std::map<int, wcstring> dico = {
{EVENT_ANY, L"any"},
{EVENT_SIGNAL, L"signal"},
{EVENT_VARIABLE, L"any"},

This comment has been minimized.

@ridiculousfish

ridiculousfish Feb 2, 2018

Member

Looks like a copy-paste error, "any" should be "variable" here.

hamon-e added some commits Feb 7, 2018

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 6, 2018

What's the status here? @ridiculousfish: Did you forget about this?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 10, 2018

It's been in my dock all this time :/

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 10, 2018

Merged as 3819437, documented as 9a5afe3. Thanks!

@ridiculousfish ridiculousfish added this to the fish-3.0 milestone Mar 10, 2018

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