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 Postman Collection Format v2 #10

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

Conversation

LucasHild
Copy link

@LucasHild LucasHild commented Oct 3, 2019

This pull requests adds support for Postman Collection Format v2.1.0. This change has been asked for in issue #9. The schema of this collection format can be found here: https://schema.getpostman.com/collection/json/v2.1.0/draft-07/docs/index.html

The default output format now is v2.1.0. I added an argument which lets the user export using v1:

flask2postman example.app --name "Example Collection" -v1 -i > example.json

@ramnes ramnes self-assigned this Oct 3, 2019
Copy link
Collaborator

@ramnes ramnes left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this great pull request and thus for your time! This is a great base to move on.

I have made a few comments, most of which are trivial. My main concern is about the coupling of the different classes with Flask that should be avoided.

Also, I am wondering if we could make the two implementations closer, by having a code that looks and feels more similar on both sides and/or by implementing a common ancestor for the Collection class.

flask2postman/__init__.py Outdated Show resolved Hide resolved
flask2postman/__init__.py Outdated Show resolved Hide resolved
flask2postman/__init__.py Outdated Show resolved Hide resolved
if self._args.folders and folder:
folder.add_request(request)
self.add_request(request)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea to move that logic here!

Can we just rename it to add_rule (without s) and modify it accordingly, so that we are not coupled to a Flask application object? We can go the simple route and pass the werkzeug rule object, but that would be even better to just pass all the arguments needed directly to the method, so that we do not close the door of supporting other frameworks than Flask someday.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, that would require to move the folder creation logic out of here, and pass the folder object as an argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we could just do like the Request class and implement a from werkzeug class method that instantiate the object at the same time. That way, we could couple the implementation with the werkzeug.Map object, without definitely enforcing Flask (or Werkzeug) as the source framework.

flask2postman/postman.py Outdated Show resolved Hide resolved
@ramnes
Copy link
Collaborator

ramnes commented Apr 19, 2020

Thanks for the work @Lanseuo! My last comment still stands but we're almost there. 🔥

@LucasHild
Copy link
Author

Hey @ramnes, thanks for your review!

I tried to solve your concerns from your last comment. Unfortunately, it's impossible to only use the werkzeug.Map object due to the fact that the properties blueprints and view_functions from the Flask app instance are required. Thus, I created a class method from_flask that allows you to instantiate the collection from the flask app instance. This has the advantage that now different web frameworks could be easier implemented.

I also renamed the Blueprint class to Folder to use a more general term that is not unique to Flask.

@ramnes ramnes removed their assignment Aug 14, 2022
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