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

Repeat less in toolset declarations #28

Open
erikrose opened this issue Jul 21, 2011 · 6 comments
Open

Repeat less in toolset declarations #28

erikrose opened this issue Jul 21, 2011 · 6 comments
Assignees

Comments

@erikrose
Copy link
Owner

Maybe stick [references to] the toolset functions within an outer function and return locals() from it, rather than writing out a big dict like peter17@19d8ad3#L0L26.

@ghost ghost assigned erikrose Jul 21, 2011
@peter17
Copy link
Collaborator

peter17 commented Jul 21, 2011

I would be glad not to have to copy/paste again and again those function names :-)

I'm not so familiar with how to deal with locals(). Can you suggest some code to do this, please?

While we are speaking about the postprocessors (html, text and raw): shouldn't they be classes and inherit from a mother class? And preprocessor as well? At the beginning, I created them as scripts to be able to load the render_* functions with an import, but now I think they should adopt a better structure.

@erikrose
Copy link
Owner Author

I assigned it to myself, but you can grab it if it saves you work. Basically,

def toolset():
def some_tool():
...

def some_other_tool():
        ...

return locals()

@peter17
Copy link
Collaborator

peter17 commented Jul 21, 2011

So simple! Thanks! I can take this issue since I'm currently working on the concerned files.

What about a class structure for the postprocessors?

@erikrose
Copy link
Owner Author

I like the idea of putting the toolset function in a class rather than in a dict. It's little less weird, and, as you say, it lets us use inheritance (though there isn't much shared functionality right now). We can put a __getitem__ method on the superclass which essentially delegates to getattr(self, whatever) so we don't have to touch the generator again.

@peter17
Copy link
Collaborator

peter17 commented Jul 24, 2011

I have been trying to implement toolsets as classes for 2 days with no success. My problem is that the functions of the toolsets are really used as "global" functions. To put them in a class, I see 3 options:

  • implement them as classic "members", which means defining function(self, node) and having an instance of the toolset, but I have to pass this instance to different Pijnu functions; otherwise, I get "TypeError: unbound method join() must be called with DefaultToolset instance as first argument (got Node instance instead)"; this makes quite a lot of changes in Pijnu and I don't really know how to easily have a toolset instance that would inherit from the 3 toolsets we have (our own in html/text/raw/preprocessor, the one in the .pijnu file and the default one from Pijnu)
  • implement them as static methods, but this requires to list them in the class with "function = staticmethod(function)" or put @staticmethod before each function; in this case we pass a reference to the class itself and not to an instance of it, but I still have problems when I want to temporarily change a method (used in preprocessor.py); quite weird
  • define some kind of "all static" class using a metaclass; I made some attempts but I also meets some problems like in the 2 previous cases

Finally, what we want is having something simple and easily define the allowed_tags and allowed_entities in the postprocessors. Maybe the current state of the code is enough for that. I will keep the class idea in mind in case I find a more efficient solution that what I tried; of course, please don't hesitate to suggest another approach, I'd be happy to try it!

@erikrose
Copy link
Owner Author

Your first alternative is the right one; the piece you're missing is to pass not SomeToolset.function but rather SomeToolset().function, so that function is bound to an instance and automatically gets the self arg. However, we don't really need the class-based approach working for the first iteration. If people want to specify a different set of allowed tags, they can pass in a custom toolset function that closes over the set of tags they want to use. We can come back to this later.

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

No branches or pull requests

2 participants