search

mrhappyasthma / IsThisStockGood

A tool for evaluating companies using the Rule #1 investing principles.
http://www.isthisstockgood.com
22 stars 10 forks source link

#28: Cleaning up a little to enable unit-testing API code. #78

Open kocielnik opened 6 days ago

kocielnik commented 6 days ago

If you feel uncomfortable with either of these changes, please let me know! I can revert such a change or give more context why I think it may make sense.

Included propositions:

  1. Replace CircleCI config with GitHub Actions config.

    • I've tried to mimic the CircleCI config one-to-one with GitHub Actions.
    • Motivation: I did not have a CircleCI account when I forked this repo. That required me to create an account with CircleCI. To use GitHub Actions, I only need the GitHub account.
      • Counter-arguments:
        1. CircleCI advertises itself as "the fastest CI service". It may be GitHub Actions are slower. Comments are welcome.
        2. Some people may not have a GitHub account, while having a CircleCI account. For them, this change would actually make things more complicated.

  2. Move requirement specification to pyproject.toml.

    • This allows using Poetry to enter a virtual env with all deps installed using one command.
    • PIP is still able to install this package from pyproject.toml - this support was added a while ago and is now pretty standard.
      • GitHub Actions CI config is an example of how similar the installation process is, while allowing the developer more flexibility (poetry install to install all deps, poetry shell to enter the virtualenv shell with them installed).
    • Counter-argument: For someone not familiar with Poetry, other workflows and tools may be much more tempting. The pyproject.toml file is pretty standard now as the format for describing packages, but the sections to be used depend on one's favourite build tool.

  3. Move code from main.py to src/server.py for testing.

    • This required moving also templates to src/templates.
      • Pro: root repo dir looks cleaner.
      • Con: I may not have full understanding of consequences of moving these templates to a subdirectory. I confirmed the page worked fine when hosted locally.

  4. Untrack .envrc.

    • This file tells your shell to enter the virtualenv immediately after cd-ing to the package directory.
    • The tool that relies on this file is called Direnv.
    • It loads the virtualenv in two seconds, unloads in less than a second.
mrhappyasthma commented 6 days ago

Thanks for all the work on this! I'll try to review it this weekend when we have some time, but generally I'm okay with any improvements to the project :)

1 - I don't have a strong preference. If we can do it as part of github actions, that should be fine. I just did Circle CI since I was familiar with it from other work.

2 - I'm not a python programmer by trade, so I never even heard of pyproject haha. But it seems to be the standard per this - https://www.reddit.com/r/learnpython/comments/1bmxe6i/whats_the_difference_between_pyprojecttoml/. So let's do it.

3 - Seems fine. I don't have a strong opinion and src/server is better named than main.

4 - Seems reasonable to me.

kocielnik commented 4 days ago

Thanks for precise feedback right on the following day!

Regarding point 1, imagining we have GitHub Actions configured, we could next try out a Google-backed recipe to deploy to Google App Engine straight from GitHub using Actions at https://github.com/google-github-actions/deploy-appengine.