moj-analytical-services / etl_manager

A python package to create a database on the platform using our moj data warehousing framework
21 stars 9 forks source link

Various refactorings/cleanups: #54

Closed xoen closed 6 years ago

xoen commented 6 years ago

Style changes:

NOTE: I'm wondering if we need to treat "RUNNING" as a special state and have the is_running() property instead of the client code using something like job.job_run_state == 'RUNNING' which is still clear.

codecov-io commented 6 years ago

Codecov Report

Merging #54 into master will increase coverage by 0.6%. The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #54     +/-   ##
=========================================
+ Coverage   73.87%   74.47%   +0.6%     
=========================================
  Files           4        4             
  Lines         823      815      -8     
=========================================
- Hits          608      607      -1     
+ Misses        215      208      -7
Impacted Files Coverage Δ
etl_manager/etl.py 58.48% <45.45%> (+1.58%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1519f8e...c127e0d. Read the comment docs.

xoen commented 6 years ago

@isichei / @r4vi I probably moved more things than I originally planned so:

  1. Double check I didn't accidentally screwed up something trivial
  2. See if you're happy with these changes, especially the style ones.

Regarding point 2, as said before, I think we should just go ahead and automatically format the code using black which gives us the big advantage of not having to think about formatting. I can format using black and PR after this, I think this first step will make the diff less harsh so still good to have it.

r4vi commented 6 years ago

@xoen nice refactoring if you're going all in on python 3.6 (fstrings) then you might want to look at pathlib for paths instead of os.path.exists stuff

also you could turn GlueJob into a context manager so client code can do something like

with GlueJob as j:
    ...do stuff
do_more_stuff()

and it'll do the cleanup when it comes out of the block