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

Make the structure more similar to other languages #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martinsson
Copy link
Owner

Partly solves #63 by putting the app creation in a create_app() function that returns the created app

The connection is not closed as I don't know the best way to do this with the multi-processing api. I don't think that's necessary as I think it's closed by the multiprocessing terminate instruction though

WDYT @emilybache @codecop ?

extract main file and put a create_app function that returns the app initialized
@emilybache
Copy link
Contributor

I don't like this at all, sorry. I don't like declaring the prices function inside the create_app function. I think it should be declared at the top level of a module. I also don't like naming the module 'main' since it is supposed to be the name of the flask app, it should at least be renamed to 'lift_pass' or similar. The database connection is created in a different thread from the request-response thread so it doesn't work at all with sqlite.

Having said all that I'm not all that experienced with Flask and I don't know the proper way to do this with Flask. It doesn't seem straightforward to create the app in one module and add routes to it in another.

@martinsson
Copy link
Owner Author

Indeed (although I did this on a project) it feels very non-python-like. In particular as python has the if __name__ == "__main__": which kind of handles this.

This allowed me to handle dependency injection in a hand-rolled manner. Here it allows both for injecting the connection and avoid the late initialisation.

I'm open for a better suggestion. But if we don't have a clear consensus of what to do I'd prefer to leave it as-is.

@codecop
Copy link
Collaborator

codecop commented Mar 16, 2023

I am very strong in favour of having projects as similar as possible because it allows me to support multiple languages at once during one session. (I had issues several times whenever a single language was different.) I did regular diffs between languages in this repo in the past and do this also on other repos. So this change makes it look similar, see my original comment. On the other hand some variation is unavoidable due to language idioms or framework needs.

  • I do not know enough Python to decide what is Pythonic-acceptable.
  • Having the entry point with named similar across all languages is favourable because then the documentation is only needed once or not at all.
  • I believe a module export is similar to a factory function - if that is the Python way because it allows such things.



if __name__ == "__main__":
app = create_app()
Copy link

@nicoespeon nicoespeon Mar 17, 2023

Choose a reason for hiding this comment

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

@martinsson unfortunately, that would not work because it returns a tuple:

tuple

If we go with this approach, I'd suggest extracting the app part like this:

Suggested change
app = create_app()
app, connection = create_app()

after

Thanks for the suggestion, it's interesting. I don't have a solid opinion to support this decision, but having experimented with this, I can say "it works". I would go for the most idiomatic approach in the language, but I don't have a lot of XP with Flask myself (it's been years) so I can't help here 😅


if __name__ == "__main__":
app = create_app()
app.run(port=3005)

Choose a reason for hiding this comment

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

We should also make it non-threaded, otherwise it doesn't work when we use the sqlite database:
thread

Suggested change
app.run(port=3005)
app.run(port=3005, threaded=False)

That's because SQLite objects can't be used in different threads, but Flask is multi-threaded by default.

@codecop codecop force-pushed the master branch 4 times, most recently from 65049f2 to 4c7caae Compare June 8, 2024 20:20
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