-
Notifications
You must be signed in to change notification settings - Fork 1
Fix runtime error #43
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 runtime error #43
Conversation
|
Sorry, nevermind, I see this already in #42 |
|
Thanks! I still need a bit to review dylan's PR before merging, we could just make this change in the meantime? |
948f2ee to
7fd9c2c
Compare
|
@SanghyunKo can you list some samples affected by your latest fix? |
|
@kdlong Oh gosh... I didn't notice that you re-opened the PR and I added another commit... Forgive me :( If you allow it, then let me explain here about the additional commit: Since |
|
No worries! IF you want you can close and open a new one. This looks like a good idea to me. Otherwise the samples fail, right? Can you collect this together with the other conditions that depend on failIfInvalidXML_ and GetError() != 0 and make the logic a bit cleaner? Right now there are several if statements that could be collected in a more clear/logical order. It might also be worth making these lines you've added into a short function named removeTrailingCharsFromHeader, for example |
|
Done. Although this may not be the most compact way, it should prevent repeating |
|
Thanks Sanghyun, this looks much better to me. One quick thing, can you run scram b code-checks and scram b code-format? |
|
Hi @kdlong, indeed there was a couple of warnings from code format check. Fixed it! |
|
Thanks! |
Fix trivial cmsRun runtime error