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

remove hardcoded compclasses #1210

Merged
merged 2 commits into from
Mar 7, 2017

Conversation

jedwards4b
Copy link
Contributor

@jedwards4b jedwards4b commented Mar 6, 2017

Removes some lingering hardcoded lists of COMP_CLASSES and uses get_values instead
Test suite: scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1170
Fixes #1171

User interface changes?:

Code review:

@gold2718
Copy link

gold2718 commented Mar 7, 2017

Did you test all of these modified scripts? I don't think scripts_regression_tests.py tests them all (e.g., ncr).

@jedwards4b
Copy link
Contributor Author

Testing of ncr indicates that it didn't work before this PR. Would you like me to jump down this rabbit hole?

@gold2718
Copy link

gold2718 commented Mar 7, 2017

Well, it sort of says that our testing program is not very robust. At the very least, you should document the status in this PR.
How about the other tests you changed? Were they all tested? Please document that.

@jedwards4b
Copy link
Contributor Author

Rabbit hole jumping ... tomorrow.

@fischer-ncar fischer-ncar merged commit 3fd75db into ESMCI:master Mar 7, 2017
Copy link

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Why was this merged before the requests were fulfilled? This PR still is not correctly documented and a new issue should be opened for the failing test.

@fischer-ncar
Copy link
Contributor

I merged it so I could run the cesm2_0_alpha06g overnight.

@gold2718
Copy link

gold2718 commented Mar 7, 2017

Why was this PR required? We always seem to be rushing around and now we have code in master which either has not been tested or at the least has tests which are undocumented.
What's the rush?

@fischer-ncar
Copy link
Contributor

I messed up. I thought Jim told be to go ahead and make a tag with this PR. It was really without
this PR.

@fischer-ncar
Copy link
Contributor

fischer-ncar commented Mar 7, 2017

I reverted this PR, but I'm can't reopen it. I think Jim needs to make a new PR. Sorry about
all of this.

@rljacob
Copy link
Member

rljacob commented Mar 7, 2017

Also you'll need to "revert the revert" when you finally put this branch on master.

@fischer-ncar
Copy link
Contributor

Which is better, to "revert the revert", or make a new PR?

@rljacob
Copy link
Member

rljacob commented Mar 7, 2017

If the PR is for the same branch, you'll still have to revert the revert. See https://git-scm.com/blog/2010/03/02/undoing-merges.html

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.

4 participants