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.

Sign up for GitHub

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

Jump to bottom

Raise more descriptive error if converting Option fails #2469

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Raise more descriptive error if converting Option fails #2469

merged 1 commit into from
Nov 29, 2021

Conversation

ZedThree
Copy link
Member

Before this change, setting an integer/BoutReal to, for example, 'true', would yield:

Cannot find variable 'true'

With this change, now the error is:

Couldn't get BoutReal from option conduction:chi = 'true': Cannot find variable 'true'

Closes #2466

I've done this off next, but it can probably be backported to master too

@ZedThree
Raise more descriptive error if converting Option fails
1eeb683 
Before this change, setting an integer/BoutReal to, for example,
'true', would yield:

    Cannot find variable 'true'

With this change, now the error is:

    Couldn't get BoutReal from option conduction:chi = 'true': \
    Cannot find variable 'true'
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

johnomotani
johnomotani approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

Nice! Looks good to me.

Is it worth using parseExpression() (or similar) to make error messages for Field3D/Field2D/FieldPerp nicer too? At the moment I think we just get the error from FieldFactory::get()->create3D(), etc.

@bendudson bendudson merged commit 4fcf070 into next Nov 29, 2021
@bendudson bendudson deleted the better-error-on-invalid-option branch November 29, 2021 19:44
@ZedThree ZedThree mentioned this pull request Dec 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendudson bendudson bendudson approved these changes

@johnomotani johnomotani johnomotani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ZedThree @bendudson @johnomotani