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

♻️ Maintenance: repo-wide cleanup of modules setups and utils #2669

Merged
merged 43 commits into from
Dec 3, 2021

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 30, 2021

What do these changes do?

Some side cleanup work from other PRs and some conventions proposals to improve maintainability.

♻️ aiohttp utils

Many tools in servicelib/aiohttp is deprecated. Slowly collecting useful utils in web-server's utils_aiohttp.py with the intention of moving them to servicelib/aiohttp as soon as all tools there are removed (e.g. openapi-core, trafaret etc). For the moment, we have centralized there:

  • create_url_for_function
  • enveloped json responses
  • Improving typing annotations (specially callables e.g. Handlers, Middlewares, etc)

♻️ Proposal: Naming convention fake and mock

  • data for testing is denoted fake and not mock to distinguish them from mock functions (i.e. using mocker fixture or unittest.mock module)
  • data for testing that is random shall be created with faker (note that faker is a locally-scoped fixture)

♻️ Proposal: setup.py

  • a single SETUP constant with all metadata and eventually extra tmp constants to avoid duplication, etc (this simple approach is taken from aiohttp)
INSTALL_REQUIREMENTS = tuple( set1 | set2) # to guarantee uniqueness

SETUP = dict(
   name = "mylibrary"
   version="1.0.2"
   ...
)

if __name__ == "__main__":
    setup(**SETUP)

Then setup metadata fields can be retrieved :

  1. as a module: importing SETUP constant and json dump it.
    python -c "from setup import SETUP; import json; print(json.dumps(SETUP, indent=1))"
  2. as a script: using setuptools.setup API
     $ python setup.py --name
     mylibrary
     $ python setup.py --version
     1.0.2 
     ...
  • 🐛 fixes versioning bump options in setup.cfg
  • Should have a single way to bump the version in a services/package. Right now there are combinations of
    • VERSION file
    • __version__
    • setup( ... version=)
  • About modules versioning:
    • which of the locations above should we bump?? I would say only one and the rest should be deduced
    • should we keep versioning everywhere or only some of the modules?
    • what is the purpose of versioning in a single-repository project? i.e. what does it bring so far?
    • (after review): use VERSION as the source of truth for each module in the repo

🩺 Linter check for python 3.10

In anticipation of future upgrades, we want to be aware ahead of possible deprecation warnings and/or syntax incompatibilities with latest releases of python. This CI job is just informative and therefore is not required for merging.

@pcrespov pcrespov self-assigned this Nov 30, 2021
@pcrespov pcrespov added this to the Meerkat milestone Nov 30, 2021
@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #2669 (4704d5d) into master (909a46a) will decrease coverage by 0.0%.
The diff coverage is 93.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2669     +/-   ##
========================================
- Coverage    78.0%   78.0%   -0.1%     
========================================
  Files         636     638      +2     
  Lines       26073   26093     +20     
  Branches     2525    2525             
========================================
+ Hits        20355   20365     +10     
- Misses       5049    5057      +8     
- Partials      669     671      +2     
Flag Coverage Δ
integrationtests 66.7% <60.3%> (-0.1%) ⬇️
unittests 73.5% <93.0%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ages/models-library/src/models_library/generics.py 91.1% <ø> (ø)
...decar/src/simcore_service_dynamic_sidecar/_meta.py 76.4% <ø> (ø)
.../web/server/src/simcore_service_webserver/utils.py 54.9% <70.0%> (+1.7%) ⬆️
...ver/src/simcore_service_webserver/utils_aiohttp.py 93.9% <91.3%> (-6.1%) ⬇️
...s-library/src/dask_task_models_library/__init__.py 100.0% <100.0%> (ø)
...ages/models-library/src/models_library/__init__.py 100.0% <100.0%> (ø)
...database/src/simcore_postgres_database/__init__.py 100.0% <100.0%> (ø)
...ackages/service-library/src/servicelib/__init__.py 100.0% <100.0%> (ø)
.../settings-library/src/settings_library/__init__.py 100.0% <100.0%> (ø)
packages/simcore-sdk/src/simcore_sdk/__init__.py 100.0% <100.0%> (ø)
... and 22 more

@pcrespov pcrespov force-pushed the maintenance/cleanup-week48 branch 3 times, most recently from 5377d4f to 7186a87 Compare December 2, 2021 17:57
@pcrespov pcrespov added changelog:♻️refactor t:maintenance Some planned maintenance work labels Dec 2, 2021
@pcrespov pcrespov requested review from sanderegg, GitHK, colinRawlings and mguidon and removed request for sanderegg December 2, 2021 18:11
@pcrespov pcrespov changed the title WIP: Maintenance/cleanup week48 ♻️ Maintenance: repo-wide cleanup of modules setups and utils Dec 2, 2021
@pcrespov pcrespov marked this pull request as ready for review December 2, 2021 18:13
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

very cool. making things more consistent is always very good! thanks for taking the time to do that.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Nice! I like the more compact and unified setup.py formats.

@pcrespov
Copy link
Member Author

pcrespov commented Dec 3, 2021

@GitHK , @sanderegg I also extended unit-test-python-linting to python 3.10 to anticipate possible deprecations and/or incompatibilities with coming releases. Note that this CI job is NOT required for merging but keep us informed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants