search

jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

fast-track: Add multipart EntityPart tests #1230

Closed jim-krueger closed 3 months ago

jim-krueger commented 3 months ago

Fixes #962

jim-krueger commented 3 months ago

@spericas I changed tabs to spaces. Was there other style or formatting issues? Thanks

spericas commented 3 months ago

@spericas I changed tabs to spaces. Was there other style or formatting issues? Thanks

Thanks. In general, I think we would prefer this:

                    EntityPart.withName("file")
                            .content("test file", xmlFile())
                            .mediaType(MediaType.APPLICATION_XML)
                            .build());

over this,

                    EntityPart.withName("file")
                    .content("test file", xmlFile())
                    .mediaType(MediaType.APPLICATION_XML)
                    .build()
                    );

in fluent calls.

However, I don't know how consistent we are on this. Maybe we can clean this up and set up some better checkstyle rules after the 4.0 release.

spericas commented 3 months ago

@jansupol Could you also review this one?

jim-krueger commented 3 months ago

@spericas Agreed, I will make those changes. Also, I'm working through an issue related to the handling of Headers that will likely result in some additional changes.

mkarg commented 3 months ago

To allow all vendors to run the new test without sacrificing their week schedule, it might be good to remove the "fast-track" marker. In the end, a missing test is not a bug, and I voted -1 for fast-tracking non-bugs.

jim-krueger commented 3 months ago

From the original Committer Conventions:
(2) For non-API, non-spec, non-javadoc changes (e.g., pom, travis, checkstyle, etc) a proposal to fast track the PR is requested at submission time. If a fast tracked PR receives 3 committer +1 votes (and no -1), it can be merged immediately provided at least 1 day has passed since its submission.

This is a TCK change and therefore "non-API, non-spec, and non-javadoc" so it can be fast-tracked. All venders would only be required to run with this after it has been placed in a release and we are weeks (or more) away from that. That being said, this PR is generating enough discussion that it is unlikely to be merged in a fast-tracked fashion.

jim-krueger commented 3 months ago

@jamezp @spericas @jansupol @mkarg Are there any more thoughts/concerns about the contents of this PR? Thanks

spericas commented 3 months ago

In general it LGTM but I'll defer to @jansupol on this one