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

Move shared python code to a shared directory #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

billsacks
Copy link
Member

I'm moving this old PR from https://github.com/NCAR/cism-development/pull/5 to this new repository.

Original comment from me, Aug 17, 2017

This demonstrates what I was thinking in terms of moving some duplicated python code to some shared location. I have implemented these changes for the dome test case. If you like this approach, the same changes would be needed in the other test cases, as well as tests/new/. Those changes should be made before merging this PR: I'm mainly opening this PR as a way to demonstrate what I was thinking.

I haven't given much thought to the naming of directories (i.e., 'lib/cism_tests_lib'), so feel free to suggest something else if you prefer.

There may be some other duplicated code that could also be moved: I haven't spent much time looking at what's in common between the different test cases.

I have implemented these changes for the dome test case; the same
changes are needed in the other test cases, as well as tests/new/
@billsacks
Copy link
Member Author

billsacks commented Jul 7, 2018

Comment from @jhkennedy Dec 1, 2017

Hey @billsacks I think this is a great idea and a ton of duplicate code could be removed.

I might suggest that it might be worth restructuring the directory entirely so that all the run*.py scripts (and any other scripts you'd like to take advantage of the shared code) are directly under $CISM/tests/. That way, you can avoid having to adjust the python paths:

https://github.com/NCAR/cism-development/blob/3bd87aacb1987e155cb49ad628d6b19885c3ccc7/tests/higher-order/dome/runDome.py#L16-L19

_TESTS_LIB_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "lib")
sys.path.append(_TESTS_LIB_DIR)

If you run a couple of these, your going to have that _TESTS_LIB_DIR in the python path many times (slowing down the python search and import process). Alternatively, you could just make sure it's not already in the path before appending it.

Most of the different test directories just contain config and data files, so they could go under a data/ directory and all shared code under a lib/ or utils/ directory to keep that top level cleaner.


This also would be a good time to ensure everything here is python 2 and 3 compatible.

I'm happy to help you with all this if you like.

@billsacks
Copy link
Member Author

Comment from me Dec 1, 2017

@jhkennedy thanks for your thoughts on this. I opened this largely to share my thoughts via a prototype. I wouldn't have time to bring this to completion for a while. So if you have the time and interest, I'd love your help.

I hadn't realized the potential issues with adding to sys.path. I remember feeling like this was an ugly solution - I just couldn't come up with anything better at the time. I'd happily support a solution that makes this cleaner.

@billsacks billsacks requested review from whlipscomb and gunterl July 7, 2018 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant