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

Relax the visibility of MockMVC DSL constructors #33778

Closed
wants to merge 3 commits into from

Conversation

Hejow
Copy link
Contributor

@Hejow Hejow commented Oct 23, 2024

Hello, I added Kotlin DSL for RestDocs.

At first, I thought this PR might be more suitable for spring-restdocs, so I was confused where to make PR.
Due to the internal constructor, I’m submitting the PR here.

I positioned the package with the following three considerations in mind

  1. The duplication issue that arises when placed in the same package as the existing Kotlin DSL.
  2. Preventing confusion for users.
  3. Reflecting the package structure of the existing Rest-docs module

I would appreciate any feedback on this PR. Thank You!!

@pivotal-cla
Copy link

@Hejow Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@Hejow Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 23, 2024
@sdeleuze sdeleuze self-assigned this Oct 23, 2024
@sdeleuze sdeleuze added in: test Issues in the test module theme: kotlin An issue related to Kotlin support labels Oct 23, 2024
@sdeleuze
Copy link
Contributor

Adding RestDocs specific code in Spring Framework is not ok, I would favor relaxing the visibility of the constructor. Could you please refine the PR accordingly and add related test, or create a related issue and close this PR?

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Oct 28, 2024
@Hejow
Copy link
Contributor Author

Hejow commented Oct 29, 2024

Thank you for ur feedback!
I guess it's much better to relaxing visibility of the constructor and let user to extend by themselves!
I will refine it! Thx 👍

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 29, 2024
@Hejow Hejow force-pushed the feature/add-rest-docs-dsl branch 2 times, most recently from d6b7cae to 31732ac Compare November 5, 2024 15:40
@Hejow
Copy link
Contributor Author

Hejow commented Nov 5, 2024

@sdeleuze Sorry that im lateee.. 😭
I refined this PR by relaxing visibility of mockMvc DSL constructor for users to customize themselves.
So, previously implemented Restdocument DSL is no longer needed, i removed it!

@sdeleuze
Copy link
Contributor

sdeleuze commented Nov 5, 2024

Could we use protected constructors instead of public ones?

@sdeleuze sdeleuze added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 5, 2024
@sdeleuze sdeleuze added this to the 6.2.0 milestone Nov 5, 2024
@Hejow
Copy link
Contributor Author

Hejow commented Nov 6, 2024

@sdeleuze I love to restrict visibility of the constructors.
But in this case, when we restrict them as protected each of DSL cannot access lamda. 🥹

p.s. thx for ur feedbacks!!! 👍

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 6, 2024
@sdeleuze
Copy link
Contributor

sdeleuze commented Nov 6, 2024

Ah yeah, indeed.

@sdeleuze sdeleuze changed the title Add RestDocumentation Kotlin DSL Relax the visibility of MockMVC DSL constructors Nov 6, 2024
@sdeleuze sdeleuze removed the status: feedback-provided Feedback has been provided label Nov 6, 2024
@sdeleuze sdeleuze closed this in 4697ae1 Nov 6, 2024
@sdeleuze
Copy link
Contributor

sdeleuze commented Nov 6, 2024

Squashed and merged, thanks!

@sdeleuze sdeleuze added the type: enhancement A general enhancement label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants