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

support templates. fixes #35 #56

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Sep 2, 2014

similar to previous PR, except now it integrates into the
main yaml file instead of a separate graphTemplates.conf

@brutasse
Copy link
Owner

brutasse commented Sep 2, 2014

Nice! It looks like a minor edit is needed in docs/api.html as well. It still mentions graphTemplates.conf.

Do you think you can add a test? Maybe with SVG rendering to make sure colors are set properly.

For greater graphite-web compatibility we should probably provide the same templates as here by default. What do you think?

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Sep 2, 2014

i personally think all the template stuff in the main yaml is obtrusive. the less template stuff in the main yaml, the better as far as i'm concerned.

maybe all that stuff could go in a separate yaml file, but let's see if anyone actually expresses an interest in having those default templates available.

@brutasse
Copy link
Owner

brutasse commented Sep 2, 2014

I meant having a templates section in default_conf in graphite_api/config.py. For people needing graphite-web's vanilla templates, they wouldn't have any configuration to do because templates would already be there (you can run graphite-api without a config file if your setup works with the defaults). And for people needing custom templates, they can simply add the section in the yaml.

But I care more about testing than providing an extensive suite of templates :)

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Sep 2, 2014

Ah, ok yeah that makes sense.
I've been looking at the testing code. This is going to take me a while to figure out what the best way is to test this properly, and i don't really have much time remaining :(

Dieter Plaetinck added 2 commits December 11, 2014 17:37
similar to previous PR, except now it integrates into the
main yaml file instead of a separate graphTemplates.conf
@Dieterbe
Copy link
Contributor Author

rebased and pushed a new fix.

@brutasse so the test should be something like the following?

    def test_render_template(self):
        whisper.create(self.db, [(1, 60)])
        from pprint import pprint
        self.app.config = {'templates': {'foo': {'linecolors': ['#aaaaaa', '#bbbbbb']}}}
        response = self.app.get(self.url, query_string={'target': 'test',
                                                        'format': 'svg'})
        # assert here that somewhere in lines we should see the first color being used for the target?
        lines = response.data.decode('utf-8').strip().split('\n')
        pprint(lines)

unfortunately the svg output doesn't contain the color anywhere :( not sure what's wrong, as the template colors work fine with png output. the lines pprint output is here: https://gist.github.com/Dieterbe/affcdc35e24c17c219f5

@Dieterbe
Copy link
Contributor Author

@brutasse bump. would love to get this merged. i attempted a unit test but i'm still stuck (see above). thanks.

@axos88
Copy link

axos88 commented Feb 2, 2016

Please merge this @brutasse

@zzl0
Copy link

zzl0 commented Nov 16, 2016

👍

@zzl0
Copy link

zzl0 commented Nov 22, 2016

@brutasse @Dieterbe What's the status of this PR?

@zzl0 zzl0 mentioned this pull request Nov 22, 2016
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.

4 participants