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

[WIP] "flexer new" command to create working examples of modules #7

Merged
merged 7 commits into from
Feb 14, 2017

Conversation

tintoy
Copy link
Contributor

@tintoy tintoy commented Jan 19, 2017

Note: This idea is a work-in-progress; I'm seeking feedback :)

Example usage:

$ flexer new -t resource_connector -n my_resource_connector

This creates a new resource connector:

$ ls ./my_resource_connector
main.py
test-data.json
run.sh

test-data.json is populated using information discovered via the CMP API (e.g. valid account and provider Ids).

You can then run ./run.sh to invoke module. run.sh simply invokes flexer run get_resources --event="$(cat ./test-data.json)".

Would love some feedback, if you have any.

@geodimm
Copy link
Contributor

geodimm commented Jan 20, 2017

Looks great, thanks a lot for the effort! When you feel like the feature is complete, please let us know so we can merge it :)
One small comment is may be to use click.echo instead of the print statement as it takes care of encoding and flushes to the appropriate file descriptors.

@tintoy
Copy link
Contributor Author

tintoy commented Jan 20, 2017

Thanks - will do :)

@tintoy tintoy force-pushed the feature/flexer-new branch from 4542fce to 199ae4a Compare January 24, 2017 06:53
@geodimm
Copy link
Contributor

geodimm commented Feb 7, 2017

Hi @tintoy ,

I was wondering if you're happy with what you have so far :) If you are, we can merge this and add further enhancements in the future if necessary.

@tintoy
Copy link
Contributor Author

tintoy commented Feb 7, 2017

Sorry! Been flat out but I'll review it first thing this morning and get back to you ASAP :)

@tintoy
Copy link
Contributor Author

tintoy commented Feb 7, 2017

If you're happy with where I've put things (layout-wise), then it should be ok to merge. I'd like to write some proper tests as the next PR though :)

Copy link
Contributor

@geodimm geodimm left a comment

Choose a reason for hiding this comment

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

Hi there,

Sorry for the late response. Lots of stuff to do around here... I've just tried it on my laptop and found a few small issues. Hope you can address them when you have time :) Also, I know it's nitpicking, but could you lint the code so we don't get lines longer than 80 chars, etc.

:param client: The CMP API client.
"""

account = client.get("/accounts")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a missing .json() call on the response object here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Sorry, I should have tested that. I'll fix it right up.

flexer/cli.py Outdated
os.path.dirname(__file__),
"templates",
module_type
)
Copy link
Contributor

Choose a reason for hiding this comment

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

if the templates_dir does not exist, the script fails with OSError: [Errno 2] No such file or directory. We should check if it exists before trying to render templates.

Copy link
Contributor Author

@tintoy tintoy Feb 14, 2017

Choose a reason for hiding this comment

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

BTW, I use PyCharm - it's always linting (PEP8), but I don't normally follow the maximum 80 chars limit. But I'm happy to do so here and will sort that out now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the idea with Python was to ask for forgiveness rather than permission (i.e. catch the error if it comes up, rather than check if it exists beforehand). I don't mind checking first though, it's what I'm used to :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's a good point. In that case the friendlier message will hint the user the program hasn't just crashed unexpectedly :) Thanks for the styling fixes too. We have no strong feelings so don't feel obliged to do it :)

@geodimm geodimm merged commit eece0a0 into ntt-nflex:master Feb 14, 2017
@tintoy
Copy link
Contributor Author

tintoy commented Feb 14, 2017

Yay!

@geodimm geodimm mentioned this pull request Feb 15, 2017
@tintoy tintoy deleted the feature/flexer-new branch March 13, 2017 00:49
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