Skip to content

Conversation

uliska
Copy link
Collaborator

@uliska uliska commented May 4, 2018

This is a suggestion for an approach to handle the situation described in #117.

What do you think, should I continue laong these lines?

This is a suggestion for an approach to handle the situation described
in #117.
retrieved from the self.handlers dictionary.
For example: self.get_handler['Command']['\\rest']
If a handler for the given element is not defined, return None."""
return self.handlers[category].get(element, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any plans to expand this to more categories besides commands?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, once I'm more familiar with it there will probably more of that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are of course other examples of multiple elif statements. But I'm not yet convinced that this is helpful as a general restructuring. The goal should be to make it easier to add new functionality and to maintain the existing code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't looked again yet at the code of this PR concretely, but I came across the issue when looking in other corners. I'm not convinced that my suggestion is the best implementation because it may seem to add some level of complexity that is contrary to the goal of making maintenance and development easier. But I'm now even more convinced that it would be good to have a dictionary based infrastructure for a number of things to replace the elif chains.

One example is the End() function that provides endings for a number of different elements.
Another example I came across is Set(). I would like to start supporting specific context properties. This would also require handlers for each supported property.

Somehow I have the feeling that it would be the most natural approach to not use the token as the dictionary key but the element's class, and then instantiate an object of a corresponding class, which would handle the request. Such a could be maintained in its own file, so there's less penalty for implementing lots of special cases (e.g. context properties, element types to be ended ...). Maybe it would even make sense to make this subclasses of the classes in ly.music.items?

However, this is more a random musing than a concrete suggestion. Maybe this won't work because there is no really common interface (i.e. does this need arguments or not, how many, etc).

OTOH I'm thinking if this couldn't be a way to provide an opportunity for people to support their \userCommands. A base path could be passed as an argument, and if there is a certain file it will be included ... No idea if that would work, but it could prove very helpful.

@uliska
Copy link
Collaborator Author

uliska commented May 10, 2018 via email

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

Successfully merging this pull request may close these issues.

2 participants