Refactor main.py #949

[Super-Fly]

Fixes #949

For the purpose of the refactoring only of main.py I have done several things:

• All of the Python code moved to app/ directory
• Helper functions are now in a separate file
• Routes are now in a separate file
• __init__ file in src/app directory for creating and configuring the Flask app
• Removed some brackets in conditions
• Reformated the config.py file to 4 spaces indentation

I didn't do any refactoring of the rest of the code because I think this should be done in separate PR

[tunetheweb]

Looks very nice, but a big change, so want to spend some time going through it today.

I did a quick test deploy and looks to be working fine with Google Cloud Platform so that's a good sign.

Some initial thoughts/observations:

• We have Talisman Security Setting still set in main.py. Is that cause it's just so small not worth moving to it's own file?
• We have cache control headers in routes.py - is this the right place for it? Other caching (for fonts) is set in __init__.py so would be nicer if they were besides each other (should we have a caching.py? Or a http_settings.py and also include security settings?)
• Should get_chapter_nextprev be a helper function?

[tunetheweb]

@SaptakS / @ibnesayeed can you cast your eyes over this too if you get a chance as think you're more familiar with Python than me!

However let's limit the scope to just setting out the file structure correctly - there's a lot of legacy code here that @Super-Fly didn't write and I agree with the point in this first comment, that other refactors are better in a future PR than making this one too big.

[ibnesayeed]

The organization of the code looks good to me. I would suggest looking at some workflows and scripts to ensure that they will continue to function as intended because some of them watch changes in certain folders. I hope they use nested wildcards to cover the new structure automatically, but there is no harm in going through them once.

[tunetheweb]

Yes once we have split it up we should look to add pytest test cases - like we have for language.py and validate.py in validate_test.py. Presume that's what you mean @ibnesayeed ?

[ibnesayeed]

Presume that's what you mean @ibnesayeed ?

I meant, we need to revisit all the places where certain paths are being watched for certain tasks to make sure they still cover what they were supposed to cover. For example:

almanac.httparchive.org/.github/workflows/generate_chapters.yml

Lines 20 to 27 in 8f51988

	paths:
	- src/**.js
	- src/**.css
	- src/**.html
	- src/content/**.md
	- src/**.py
	- src/**.json
	- src/requirements.txt

[tunetheweb]

Good catch - @Super-Fly can you add src/app/**.py to that file to this PR? - see below

[tunetheweb]

Actually skip that - not needed, that's covered by src/**.py

[ibnesayeed]

No, it does not need to be updated, as ** covers all nested directories, but I just wanted to point it out that we need to revisit such occurrences to ensure things are in order.

[Super-Fly]

I didn't move Talisman out of the main.py because his instance is required in the same file to turn HTTPS off but basically we need it only in routes.py

In order to fix that, I was thinking to create better debug detection, maybe to have .env file for development where we can override defaults, something like that

With that fixed, we can move Talisman setting directly in routes.py and in configure() to set HTTPS on/off

As for the add_header() cache, for me it seems logical to have all route related stuff somewhere there :)

Maybe we can take out all numbers for caching in config.py?

What do you think?

[tunetheweb]

I didn't move Talisman out of the main.py because his instance is required in the same file to turn HTTPS off but basically we need it only in routes.py

Ah yes I see that now. Still, to me it seems like it's setup or config, so should really be part of __init__.py or config.py. No way to have talisma available as an importable variable or global? Kind of like the way we do with some of the config (like config_json)? Tried it myself and failed miserably but my python isn't so hot.

In order to fix that, I was thinking to create better debug detection, maybe to have .env file for development where we can override defaults, something like that

With that fixed, we can move Talisman setting directly in routes.py and in configure() to set HTTPS on/off

As for the add_header() cache, for me it seems logical to have all route related stuff somewhere there :)

Well it depends whether you see add_header as route-related, or config. To me, as it's not route specific, it's config so I'd probably move it into __init__.py to be honest - similar to the fact we have app.config['SEND_FILE_MAX_AGE_DEFAULT'] = 10800 in there.

I was thinking route.py should literally be a list of @app.route definitions. Not sure about @app.errorhandler - they're kind of a special route so maybe OK to stay in there, or maybe should be in their own errors.py file?

Also is there any way to list the routes, without having them within a def configure? Just thinking it would be easier to read and manage if not all indented like the way they are now. The flask documentation for larger apps shows some way to do this, but does warn about circular references. Again that might depend on being able to define thinks like app and talisman instances as importable or globally available.

WDYT?

[Super-Fly]

Really like this kind of PR by the way :)

Got your points, let me look through it again and will push something

btw I didn't like wrapping the routes in function but was stuck for a while and wanted to see some comments

[tunetheweb]

BTW I managed to move config.py to the app folder!

Check out these changes necessary to make that work: https://github.com/Super-Fly/almanac.httparchive.org/compare/main...bazzadp:refactor_py?expand=1

[tunetheweb]

These are the diffs for config.py itself (as well as moving it obviously!) as that may not be as obvious from above link:

% diff config.py app/config.py
8c8
< ROOT_DIR = os.path.dirname(os.path.abspath(__file__))
---
> ROOT_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)),'..')
73c73
<     for root, directories, files in os.walk('config'):
---
>     for root, directories, files in os.walk(ROOT_DIR + '/config'):

[rviscomi]

Love this! Just a few naming bikesheds.

[tunetheweb]

This looks fantastic now! Thanks so much!

One small extra ask - can you add yourself to src/config/2020.json in the developers team so you get credit on our Contributors page? It's in alphabetical order by first name.

[tunetheweb]

@Super-Fly could you look at some suggested changes in Super-Fly#2 and then we can merge this?