canonical / snapcraft.io

The official website's repository for the Snap store
https://snapcraft.io/
Other
145 stars 108 forks source link

Make snapcraft.io as a python module #554

Closed tbille closed 6 years ago

nottrobin commented 6 years ago

I'm not sure what you intended by a "Python module". I don't really think snapcraft.io has any business being a python package, with a setup.py etc.

I think all that you mean is to put the python files into a subfolder. You've talked previously about putting it into a folder called snapcraftio.

My suggestion would instead be to follow the standard that we've used for Django sites of keeping all application code in a folder called webapp. Reasons for this:

  1. There's no need to repeat the project name as its already the repository name (snapcraft.io/snapcraftio is unnecessary repetition, snapcraft.io/webapp is more informative)
  2. webapp at least partially describes the fact that the folder contains application code
  3. We can have a consistent structure between our projects, so our devs can know where to find the code.

While you're moving files around, I would also suggest:

  1. That you remove the modules folder (this may have already been your intention), and put those files at the top level of webapp/ - this is because "modules" really means nothing useful in Python. Basically all files or folders are modules.
  2. That you invert the structure of your public and publisher folders, so that instead of public/api.py and public/views.py you have api/public.py and views/public.py. This is because in an MVC app, a top-level directory called "views" is pretty common (e.g. see Django), and it groups the "api" code together, which I think is a better coupling. One day soon I hope we'll publish your API code as a snapcraft_api package on PyPi.

@tbille let me know if you disagree with any of these suggestions and we'll discuss it.

WillMoggridge commented 6 years ago

I believe that is the intention, to move items into a folder to namespace them.

I do prefer naming the folder with a unique module name related to that project, it is the defacto standard in the Python world. Though, we have been using webapp across our apps and it isn't particularly bad way of doing it as far as I can see.

For point 2 of the moving files around, I would think it is nicer to keep sections of the site as self contained modules as much as possible. Then those modules have an MVC style as they kind of do now. I do agree that I think the API stuff would be really nice as a separate module, which we could publish at a later date.

tbille commented 6 years ago

There's no need to repeat the project name as its already the repository name (snapcraft.io/snapcraftio is unnecessary repetition, snapcraft.io/webapp is more informative)

I understand this point, but does it really matter? I know that this is our way of doing in our Django projects. But why don't we want to follow a bit how it's done in the Flask world:

That you remove the modules folder (this may have already been your intention), and put those files at the top level of webapp/ - this is because "modules" really means nothing useful in Python. Basically all files or folders are modules.

:+1:

That you invert the structure of your public and publisher folders, so that instead of public/api.py and public/views.py you have api/public.py and views/public.py. This is because in an MVC app, a top-level directory called "views" is pretty common (e.g. see Django), and it groups the "api" code together, which I think is a better coupling. One day soon I hope we'll publish your API code as a snapcraft_api package on PyPi.

I agree with this point that we could create a snapcraft_api module.

But it seems much more simplier to have the view, the logic and the api(for now) in the same file. It creates less dependencies between folders and make it much more readable to have this structure:

my_module
├── api.py
├── __init__.py
├── logic.py
└── views.py

In s.io we clearly have 2 applications (in module sens): public and publisher which uses different endpoints, have different behaviours and are completely independent from each other. In the future we are going to add some more features to both of those modules. I think we will have rework the internal structure of the modules (see Dumb example). By doing your solution I am afraid that it will couple a lot the public and the publisher modules, and will hard to restructure in the future.

Dumb example (with API as a module):

publisher
├── build
│   ├── logic.py
│   └── views.py
├── __init__.py
├── logic.py
├── metrics
│   ├── logic.py
│   └── views.py
└── views.py
nottrobin commented 6 years ago

I'm fine with keeping the public and publisher separation the way round you've done it if that's what you both think is best. So after discussing in dev catch-up, I think all that's then needed for this issue is: