uwefladrich / scriptengine-tasks-ecearth

ScriptEngine tasks for the EC-Earth model
GNU General Public License v3.0
3 stars 3 forks source link

Refactor: use tempdir/pathlib instead of os where suitable #61

Closed valentinaschueller closed 2 years ago

valentinaschueller commented 2 years ago

fix #49

uwefladrich commented 2 years ago

Good to see this!

May I jump in with an early suggestion: There are two possible patterns to use pathlib.Path:

import pathlib
# use pathlib.Path()

or

from pathlib import Path
# use Path()

The former is used in 228a8ae, however, I would vote to use the latter. Two reasons:

A possible counter argument would be if we used something other than Path from pathlib. But even then I would vote for the somewhat verbose

import pathlib
from pathlib import Path
# use Path()
# use pathlib.SomethingElse()

with the understanding that this would be a rare case.

valentinaschueller commented 2 years ago

however, I would vote to use the latter

I agree, this is implemented in 17101ab. I finally hunted the bug down and can now proceed to update the other instances of paths/os

valentinaschueller commented 2 years ago

I think this is ready to go! os is left in some places but for a reason:

I applied black + isort for everything and only touched the SI3 tasks in places where it should not lead to merge conflicts with PR #60

valentinaschueller commented 2 years ago

@uwefladrich regarding 1a73bf4. Do you run isort before running black? the formatting of imports was done by isort and I wasn't sure what should be the "final" tool to call

uwefladrich commented 2 years ago

Do you run isort before running black?

Yes, isort first and then black. The reason is that black should have the last word about style. isort should just sort the imports.

valentinaschueller commented 2 years ago

In the tests, I would suggest to use the tmp_path fixture instead of tmpdir, see test_file_handling.py for an example. tmp_path is a Path object and you can avoid part of the string handling for path names.

Fixed in 51fcc87. Note: I kept tmpdir in three tests since there, .mkdir().join() and .write() are used and solving it with PosixPath leads to longer and less readable code