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

action #2996 - ensure well formed json for needles #164

Merged
merged 3 commits into from Feb 6, 2015

Conversation

psladek
Copy link
Contributor

@psladek psladek commented Feb 6, 2015

explicit hash checking via decode_json() import

@coolo
Copy link
Contributor

coolo commented Feb 6, 2015

test suite is broken

}
if (!exists $area->{'width'} ) {
$errorh{'message'}="area without width";
return \%errorh;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you returing hashref if you only use that for storing the string? Why not return the error message itself?
And it seems tidy did not catch this, but AFAIK we use spaces around = everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idea was to return a richer structure, in the end the message was enough. Not really a problem as it is now, and allows easy extension if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it looks rather criptic and thus it is kind of problem for me. Can be easily changed to hashref if needed in the future. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, if this is cryptic, I do not know how to call the rest of most of the openQA code... but ok, no problem to change it.

@coolo
Copy link
Contributor

coolo commented Feb 6, 2015

LGTM

aaannz pushed a commit that referenced this pull request Feb 6, 2015
action #2996 - ensure well formed json for needles
@aaannz aaannz merged commit ca52d84 into os-autoinst:master Feb 6, 2015
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.

None yet

3 participants