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

Fix suite freeze on non-existent xtrigger - 7.8.x #3059

Merged
merged 2 commits into from
Apr 3, 2019

Conversation

dwsutherland
Copy link
Member

Port of pull/3056

@kinow
Copy link
Member

kinow commented Apr 2, 2019

Travis is unhappy because of an error with pycodestyle I believe.

lib/cylc/config.py:2107:33: E128 continuation line under-indented for visual indent
lib/cylc/config.py:2110:29: E128 continuation line under-indented for visual indent
The command "pycodestyle --ignore=E402,W503,W504 lib/cylc lib/Jinja2Filters/*.py lib/parsec/*.py $(grep -l '#!.*\<python\>' bin/*)" exited with 1.

(this error appears near the top of the output, which is misleading, as you normally expect the error near the bottom of the file/output)

@dwsutherland
Copy link
Member Author

(this error appears near the top of the output, which is misleading, as you normally expect the error near the bottom of the file/output)

I couldn't see that log detail at all, was just the working info.. anyways fixed.

@matthewrmshin matthewrmshin added this to the next-release milestone Apr 2, 2019
@matthewrmshin matthewrmshin added the bug Something is wrong :( label Apr 2, 2019
@matthewrmshin
Copy link
Contributor

Given the final comments under #3056, do we need to raise a new PR against master for a test and back port that here?

@matthewrmshin
Copy link
Contributor

Happy to merge this otherwise, and have separate back port PR for test.

@matthewrmshin
Copy link
Contributor

Is that #3060?

@dwsutherland
Copy link
Member Author

dwsutherland commented Apr 2, 2019

Given the final comments under #3056, do we need to raise a new PR against master for a test and back port that here?

correct

Not here sorry; New test PR port of 3060 to come post merge of this one.

@kinow
Copy link
Member

kinow commented Apr 2, 2019

Not here sorry; New test PR port of 3060 to come post merge of this one.

Yup, I merged it too quickly, sorry. So now we will have yet another backport pull request. Mea culpa 😞 I'll submit it once both get merged.

@dwsutherland
Copy link
Member Author

Not here sorry; New test PR port of 3060 to come post merge of this one.

Yup, I merged it too quickly, sorry. So now we will have yet another backport pull request. Mea culpa 😞 I'll submit it once this gets merged.

Worked out well, since your test construction > mine ATM (>.<) ... Gave me a chance to use yours as a tutorial.

@dwsutherland dwsutherland merged commit 1bed60a into cylc:7.8.x Apr 3, 2019
raise SuiteConfigError("ERROR, xtrigger function "
"not callable: "
"%s" % xtrig.func_name)
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, just realized you have ModuleNotFoundError on master, and the parent class ImportError on 7.8.x. I think we better use the parent ImportError on master too @dwsutherland . WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kinow - Yes, as python3 gave me ModuleNotFoundError, but python2 produced ImportError and the former is not recognised. I didn't try ImportError in python3 (Is it back compat?), and figured both have diverged anyway.. Is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting (another hole in my Python 3 knowledge!).

Copy link
Member

Choose a reason for hiding this comment

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

I believe ImportError is the parent class of ModuleNotFoundError.

Interesting (another hole in my Python 3 knowledge!).

Me too! I thought at least this part was consistent in both. But catching the parent class should be fine, though better to confirm if that works. Perhaps the unit tests could be used to validate that it works?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe ImportError is the parent class of ModuleNotFoundError.

Well I guess the child doesn't exist in python2 ... Consistency aside, would it have been better to use the parent?

Also does consistency matter between 7.8.x and 8? at some point we'll stop back porting changes? (as I guess it matters for that)

Copy link
Member

Choose a reason for hiding this comment

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

Good point. If that works as-is, all good then I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants