[WiP] [JENKINS-28728]: Support Pretty JSON outputs #19

recena · 2015-06-06T22:04:02Z

- Introduce a universal method to manage return formats
- Update all remote API commands
- UI hyperlinks should return the data in pretty format by details ("raw data" links from container and image pages)
- Built-in API documentation should describe the option (1 global section would be enough)
- Several words in README.md

recena · 2015-06-06T22:23:55Z

@oleg-nenashev @jtnord Your review would be useful

oleg-nenashev · 2015-06-07T07:11:44Z

src/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java

Please avoid empty-line changes

oleg-nenashev · 2015-06-07T07:15:32Z

The behavior should be optional, because non-pretty JSONs are preferable for the automated processing. I would propose to pass the mode via an additional enum-based parameter (d.g. mode=json, json-pretty, xml), which would alter the behavior of toJSONResponse() (could be also renamed)

…ry Parameter

…ure that maps are serialized the same way

recena · 2015-06-07T14:11:21Z

@oleg-nenashev @jtnord Your review would be useful, thanks in advance.

oleg-nenashev · 2015-06-07T14:52:14Z

src/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java

I would throw an error in the case of the unsupported format

oleg-nenashev · 2015-06-07T14:58:00Z

👍 for the concept, noted minor improvement proposals in the code

recena · 2015-06-07T16:22:03Z

@oleg-nenashev I've reviewed your last recommendations.

oleg-nenashev · 2015-06-08T06:49:20Z

src/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java

I would also keep it in enum class, but it's optional at the current stage

oleg-nenashev · 2015-06-08T06:56:01Z

@recena Thanks a lot for the adjustments. In the pull-request description I've noted several UI/UX-related things, which would be useful to address in this pull request. BTW, we can do it later.

…uires pretty output

recena · 2015-06-08T08:15:59Z

@oleg-nenashev I've reviewed the last adjustments.

rsandell · 2015-06-08T13:29:57Z

src/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java

why the object type and not the primitive?

@rsandell Done.

rsandell · 2015-06-08T19:15:59Z

public boolean isPretty() other than that :+1

jtnord · 2015-06-09T11:24:47Z

...c/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java

and the format of format is...

recena · 2015-06-21T00:00:10Z

@oleg-nenashev This was a previous step before adding new documentation related to new formats. What do you thing about it?

recena · 2015-06-21T10:17:04Z

@oleg-nenashev I don't know what you mean with UI hyperlinks should return the data in pretty format by details ("raw data" links from container and image pages). Could you put an example?

oleg-nenashev · 2015-06-26T16:39:55Z

...urces/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction/_api.jelly

Weak 👎 on this CSS in the plugin. If you want to add specific highlighting, it's better to create appropriate controls in Jenkins core. These styles may be an interim solution only

@oleg-nenashev

Could you tell if exist another plugin that includes its own API documentation?

This change is not only highlighting, it also includes semantic in the markup

There are a lot of plugins that includes its own CSS style in jelly templates

If Jenkins Core includes a proposal to attend this, I'll remove the local CSS styles

Sorry, I've missed the comment somehow. I propose to decouple the help formatting to a separate pull request, because it requires additional discussion and does not block the main proposed functionality.

@oleg-nenashev Before revert my work (and throw my time to the trash), I'd like to know your technical reasons why you reject this.

@recena
Just to clarify... I don't propose to throw your work to trash. I just propose to submit it as a separate pull request in order to review it separately from the functional changes.

oleg-nenashev · 2015-08-06T07:15:13Z

@recena
Let's put the discussion on hold for several days. After that I can come up with proposals regarding the API documentation layout changes. Your proposals are very useful, but I would like to somehow move them to the Jenkins core's level.

oleg-nenashev · 2015-08-07T06:05:17Z

README.md

I don't see a need in such change, but I'm OK to replace it.

@oleg-nenashev I don't remember why, but I had to change this for any reason.

oleg-nenashev · 2015-08-07T06:14:21Z

Hi @recena
I'm OK with replacing the entire text in API text files. The only thing I'm concerned about is the usage of internal CSS files without style definitions. It causes a technical debt when Jenkins core starts supporting API documentation templates out-of-the-box. Since I have not found time to propose a patch to Jenkins core, I finally conclude it can be merged as is and reworked later.

So, now we just need to finish the action items, merge the PR with the current branch and then squash commits.

oleg-nenashev · 2015-08-07T06:16:14Z

...urces/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction/_api.jelly

The description below describes the structure of the json field. The new formatting may be quite confusing, a text should be adjusted at least

@oleg-nenashev I'll take a look.

recena · 2015-08-07T18:34:02Z

@oleg-nenashev As I said in above comment, if Jenkins core provides a global proposal for that, I'll send a new PR to review it.

recena · 2015-08-07T18:44:19Z

@oleg-nenashev I hope to send new modifications as soon as possible

oleg-nenashev · 2017-01-11T13:11:02Z

CC @alexanderrtaylor. BTW I feel it's stuck

recena · 2017-10-02T11:11:39Z

Two years ago...

alexanderrtaylor · 2018-02-13T14:19:15Z

Sorry @recena I am looking it over now. Can you re-trigger the tests though? They are still showing as "pending"

alexanderrtaylor

Pending a successful rebuild this Looks good and works for me locally

recena · 2018-02-13T16:50:32Z

@alexanderrtaylor I'll take a look later.

Related to [JENKINS-28728]: Support Pretty JSON outputs

2f09450

oleg-nenashev reviewed Jun 7, 2015
View reviewed changes

recena added 5 commits June 7, 2015 11:50

Removing empty-line changes

fee3e43

Included support to different response formats through a optional Que…

f2a6a57

…ry Parameter

The QueryParam format should be no required

bcb9be3

Added this feature ORDER_MAP_ENTRIES_BY_KEYS in jsonRoundTrip to be s…

6dc9f92

…ure that maps are serialized the same way

Removing empty-line changes

ac87498

oleg-nenashev reviewed Jun 7, 2015
View reviewed changes

When a format is not supported, an IllegalStateException is thrown

5a5d62c

oleg-nenashev reviewed Jun 8, 2015
View reviewed changes

More consistency ResponseFormat (enum type) including if a format req…

16c44c1

…uires pretty output

rsandell reviewed Jun 8, 2015
View reviewed changes

oleg-nenashev added the enhancement label Jun 8, 2015

Some revisions suggested by Robert Sandell

603ed20

jtnord reviewed Jun 9, 2015
View reviewed changes

...c/main/java/org/jenkinsci/plugins/docker/traceability/core/DockerTraceabilityRootAction.java Outdated

Copy link

Member

jtnord Jun 9, 2015

Choose a reason for hiding this comment

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

and the format of format is...

recena added 4 commits June 9, 2015 19:19

Javadocs reviewed

3f46457

Javadocs revisions

fb5e79f

Merge remote-tracking branch 'upstream/master'

181472c

Styling API Documentation

8d7ef1f

recena added 2 commits June 21, 2015 11:30

Included documentation related to the new queryParam: format

505f594

Minor revisions

ee94c15

oleg-nenashev reviewed Jun 26, 2015
View reviewed changes

recena changed the title ~~Related to [JENKINS-28728]: Support Pretty JSON outputs~~ [JENKINS-28728]: Support Pretty JSON outputs Jul 5, 2015

oleg-nenashev changed the title ~~[JENKINS-28728]: Support Pretty JSON outputs~~ [WiP] [JENKINS-28728]: Support Pretty JSON outputs Aug 6, 2015

oleg-nenashev reviewed Aug 7, 2015
View reviewed changes

[JENKINS-28728] I don't want to lose info

a20b024

recena force-pushed the master branch from ce8d6c1 to a20b024 Compare October 19, 2015 16:17

Merged

123042e

alexanderrtaylor approved these changes Feb 13, 2018

View reviewed changes

Uh oh!

[WiP] [JENKINS-28728]: Support Pretty JSON outputs #19

Are you sure you want to change the base?

[WiP] [JENKINS-28728]: Support Pretty JSON outputs #19

Uh oh!

Conversation

recena commented Jun 6, 2015

Uh oh!

recena commented Jun 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Jun 7, 2015

Uh oh!

recena commented Jun 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Jun 7, 2015

Uh oh!

recena commented Jun 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Jun 8, 2015

Uh oh!

recena commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsandell commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recena commented Jun 21, 2015

Uh oh!

recena commented Jun 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Aug 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Aug 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recena commented Aug 7, 2015

Uh oh!

recena commented Aug 7, 2015

Uh oh!

oleg-nenashev commented Jan 11, 2017

Uh oh!

recena commented Oct 2, 2017

Uh oh!

alexanderrtaylor commented Feb 13, 2018

Uh oh!

alexanderrtaylor left a comment

Choose a reason for hiding this comment

Uh oh!

recena commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels