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

Corrected JSON structure of RabbitMQ message #66

Merged
merged 2 commits into from
Mar 7, 2017
Merged

Corrected JSON structure of RabbitMQ message #66

merged 2 commits into from
Mar 7, 2017

Conversation

TWestling
Copy link
Member

The found indications was only a single indication,
it is now a list.
Also, the ID of the failure cause was the key of each
failure cause, it has been moved inside the failure cause
structure.

The found indications was only a single indication,
it is now a list.
Also, the ID of the failure cause was the key of each
failure cause, it has been moved inside the failure cause
structure.
for (FoundIndication ind : foundFailureCause.getIndications()) {
foundIndicationsJSON.put("pattern", ind.getPattern());
foundIndicationsJSON.put("matchingString", ind.getMatchingString());
JSONObject foundIndicationJSON = new JSONObject();
Copy link
Member

Choose a reason for hiding this comment

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

nit With just the difference of an s in the variable name it makes it a bit hard to read.

Copy link
Member

Choose a reason for hiding this comment

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

And the use of JSON in the name feels very redundant.

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 can agree on the naming, I'll add a new commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rsandell I feel like having JSON in the name makes it a little more easy to read since I am dealing with actual foundFailureCauses as well as json objects containing information from failurecauses/indications. I'll keep the "JSON" but make it more clear which ones are arrays and which ones are not.

Copy link
Member

@rsandell rsandell left a comment

Choose a reason for hiding this comment

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

Just nits

The names of variables representing various parts of the JSON
structure weren't really clear. I have chosen better names for these.
@TWestling TWestling merged commit c9434f4 into jenkinsci:master Mar 7, 2017
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.

2 participants