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

Feature/jstassi/#98 convert scripts to python3 #99

Merged
merged 2 commits into from
Aug 20, 2022

Conversation

gmao-jstassi
Copy link
Contributor

No description provided.

Merge branch 'develop' into feature/jstassi/#98-Convert_scripts_to_Python3
@gmao-jstassi gmao-jstassi requested a review from a team as a code owner August 11, 2022 19:14
@gmao-jstassi gmao-jstassi added the 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label Aug 11, 2022
@gmao-jstassi
Copy link
Contributor Author

Ricardo, these updates are utility programs that do not have any impact on DAS processing.

Copy link
Contributor

@rtodling rtodling left a comment

Choose a reason for hiding this comment

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

Joe,

I might have been distract when this come into the develop - I don't actually recall ever approving this stuff ... but that's besides the point.

I can't approve this as is. This needs major cleaning.

Why are there copies of RC files from ETC in the input directory of the python utils you added - I know this has been the case for a while, but I don't recall approving this. Regardless, it cannot be as is

  • What is there an outdir w/ a single README file ... this seem like something that should be created when user run you software somewhere else outside the source code ... this dir should not be here.

  • what are all these files? Why are some of these files copy of each other? e.g.: check_summary.satbang_full_rst check_summary.satbang_full_rst.orig are identical. Are these files output of a run using your software ... why should they be in the repo at all.

Sorry, but I think this needs some attention before it can be taken into the tag.

@gmao-jstassi
Copy link
Contributor Author

Ricardo, all the files that you mention are in the unittests subdirectory. These files are not included in the build. Their only purpose is to test the Python scripts in the GSI_GridComp/etc directory, to verify that the scripts give expected output after being modified. That seems like a useful thing to have. The unittests/outdir subdirectory with its README file were previously in the repository but have been deleted with this PR.

@rtodling
Copy link
Contributor

Why can't you design your test so that it runs off the RC files in the ETC directory of the installation? Why should you copy files from gsi/etc into your unit-test env. Sorry, but I still think there is a level of clean up to be done here. You haven't explained to me why you need files that identical to be in the repo w/ different names!

@gmao-jstassi
Copy link
Contributor Author

gmao-jstassi commented Aug 15, 2022 via email

Copy link
Contributor

@rtodling rtodling left a comment

Choose a reason for hiding this comment

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

Ok - I might still have my reservations about some of the things here, but since they: (i) don't affect anything in a run; (ii) are a move forward in terms of python - I think the best is to approve this and work on some cleanup later.

@rtodling rtodling merged commit 651401a into develop Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants