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

[Stalled] Provide an Elsa class that can be used programmatically #39

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@hroncok
Member

hroncok commented Jun 28, 2017

Related: #32

This is still very much work in progress, currently:

  • There is a Elsa class people can use to do freeze(), serve() and deploy()
  • cli(...) is now a shortcut to Elsa(...).cli()
  • all integration tests pass without modification

However:

  • a lot of stuff inside still prints and exists (notably deploy) - this is not acceptable with library API

I'd like an early feedback if possible.

@hroncok hroncok requested review from Sanqui and encukou Jun 28, 2017

Arguments:
* app: and instance of Flask WSGI application

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

*an instance of a Flask WSGI application
(typo and missing article)

base_url = (base_url or self.base_url or
self.app.config.get('FREEZER_BASE_URL'))
if not base_url:
raise ValueError('No base URL provided!')

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

I think errors don't use punctuation by convention

warnings.filterwarnings('error',
category=flask_frozen.FrozenFlaskWarning)
print('Generating HTML...')

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

Should an API like this print on its own? Perhaps logging as INFO would be better, since a logger is already in use. Alternatively, a verbose parameter.

This comment has been minimized.

@hroncok

hroncok Jun 28, 2017

Member

Nope, it definitively should not, I say that in the PR description. A logger is already in use?

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

I missed that, sorry. Also, I didn't realize warnings is not directly related to logging. My mistake.

print('Generating HTML...')
self.freezer.freeze()
def freeze_fail_exit(self, path=None, base_url=None):

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

Is this useful? Usually libraries don't quit on their own.
Could it be collapsed into a parameter of freeze (like exit_on_failure)?

This comment has been minimized.

@hroncok

hroncok Jun 28, 2017

Member

I need to do it twice, so I created a function. Maybe it should be underscored, not to be part of the API.

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

I can only see you using it once right now. If it's only used in the CLI, I'd say it could be defined in there, but your CLI is intertwined with the Elsa class anyway (probably not really a bad thing).
Underscore should be fine too.

This comment has been minimized.

@hroncok

hroncok Jun 28, 2017

Member

I can only see you using it once right now

Good catch. Now I use it twice :D

def serve(self, port=DEFAULT_PORT):
"""Serve the frozen app using the freezers ability to serve what was
frozen"""

This comment has been minimized.

@Sanqui

Sanqui Jun 28, 2017

Member

*freezer's

@hroncok

This comment has been minimized.

Member

hroncok commented Jun 28, 2017

Should the cli() thing be decoupled to not be part of the class?

@Sanqui

This comment has been minimized.

Member

Sanqui commented Jun 28, 2017

I'm honestly not sure. I probably would have put it outside myself, but I don't think the current state of things is wrong.

@hroncok hroncok removed the request for review from encukou Jun 28, 2017

@hroncok

This comment has been minimized.

Member

hroncok commented Jun 28, 2017

OK, let me work on this as is, will try to fix all the printing and error reporting and change it so that only the actual CLI is talking. Will also try to add some unittests, since now we are defining an API.

@Sanqui

This comment has been minimized.

Member

Sanqui commented Jun 28, 2017

Sounds good to me!

@hroncok

This comment has been minimized.

Member

hroncok commented Sep 14, 2017

BTW I was thinking about this more and I think making cli() a separate function once again and just create an Elsa instance on each command will be more maintainable.

@hroncok hroncok changed the title from WIP: Provide an Elsa class that can be used programmatically to [Stalled] Provide an Elsa class that can be used programmatically Mar 25, 2018

I want to block this somehow, yet I cannot add my own "change required" review

@hroncok hroncok added the stalled label Jul 30, 2018

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