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

Absolute template directories should work without Morepath having to know the module path #299

Closed
href opened this Issue Mar 11, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@href
Copy link
Member

href commented Mar 11, 2015

The documentation states that it should be possible to either return a relative or an absolute path for the template_directory directive.

The code indicates however that this doesn't work:

assert self.attach_info is not None

I also couldn't find a test that checks that. Am I missing something?

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Mar 13, 2015

Perhaps there's no test, but it ought to work. Try this in Python:

  os.path.join('/original', '/new')
@href

This comment has been minimized.

Copy link
Member

href commented Mar 13, 2015

That's true. I looked at it again and my issue is ill-defined. 😓

I do have the problem that I can't return an absolute path in tests, because they happen in a pytest folder without __init__.py. Which results in self.attach_info being None.

See https://github.com/OneGov/onegov.core/blob/a609/onegov/core/tests/test_templates.py#L53

That assertion threw me off here. I think it's not necessary if the path given is already absolute, no?

@href href changed the title Absolute template directories don't work Absolute template directories should work without Morepath having to know the module path Mar 13, 2015

@href

This comment has been minimized.

Copy link
Member

href commented Apr 13, 2015

I'm back and I would like to fix this, but I wanted to get your feedback first :)

assert self.attach_info is not None
asserts that attach_info is not None:

    def perform(self, registry, obj):
        directory = obj()
        assert self.attach_info is not None
        directory = os.path.join(os.path.dirname(
            self.attach_info.module.__file__), directory)
        registry.register_template_directory_info(
            obj, directory, self.before, self.after, self.app)

This seems unnecessary, if the directory is already an absolute path, since the first parameter of os.path.join is simply ignored in this case.

So I would rewrite this as follows:

    def perform(self, registry, obj):
        directory = obj()
        if not os.path.isabs(directory):
            assert self.attach_info is not None
            directory = os.path.join(os.path.dirname(
                self.attach_info.module.__file__), directory)
        registry.register_template_directory_info(
            obj, directory, self.before, self.after, self.app)

What do you think?

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 13, 2015

Change sounds good to me! As long as the tests pass feel free to check it into master.

Though a note about an earlier comment you made: you said attach_info is None as they happen in a pytest folder without __init__.py. But in tests it's usually None as no Venusian scanning takes place in the first place (only tests that explicitly scan have it).

P.S. Great, welcome back! I hope you had a good trip!

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 13, 2015

Put a comment about what you did and why in CHANGES.txt too, please. You can refer to this issue in it. Not sure whether an (API) doc update is needed to describe this use case.

@href

This comment has been minimized.

Copy link
Member

href commented Apr 13, 2015

Okay, thanks. I'll do that.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 13, 2015

Exciting to see someone besides me put things into master! 👍

@href href closed this in 8bd7e52 Apr 13, 2015

@href

This comment has been minimized.

Copy link
Member

href commented Apr 13, 2015

There you go, the API docs didn't need updating as it already mentions that paths are relative to the module and there's no change in that behavior.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 13, 2015

Hm, do the docs also say you can put in an absolute path?

@href

This comment has been minimized.

Copy link
Member

href commented Apr 13, 2015

They do:

The decorated function gets no argument and should return a

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 13, 2015

Oh, I must've done that already back then. :) Thanks for double checking!

@href

This comment has been minimized.

Copy link
Member

href commented Apr 13, 2015

Welcome ;)

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