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.

Sign up for GitHub

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

Jump to bottom

Added property config query-config-enabled #1366

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Added property config query-config-enabled #1366

merged 1 commit into from
Feb 7, 2023

Conversation

mfpc
Copy link
Contributor

@mfpc mfpc commented Jan 30, 2023

Added property config query-config-enabled

@mfpc mfpc force-pushed the query-config-enabled branch from eb599d0 to a487570 Compare January 30, 2023 13:20
@mfpc mfpc mentioned this pull request Jan 31, 2023
Closed
MikeEdgar
MikeEdgar reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

@mfpc there is a formatting issue. You should be able to just run mvn package locally and commit the formatting changes to resolve this.

@mfpc mfpc force-pushed the query-config-enabled branch from 2795c05 to 9195684 Compare February 2, 2023 13:13
@mfpc
Copy link
Contributor Author

mfpc commented Feb 2, 2023

Hi,

I have a question, when I use this code DEFAULT_OPTIONS.put(Option.queryConfigEnabled, null); my test fail assertTrue(s.contains("queryConfigEnabled"));

If I change to DEFAULT_OPTIONS.put(Option.queryConfigEnabled, "false"); it works. Should I fix this or remove this test?

Thank you

gastaldi
gastaldi reviewed Feb 2, 2023
View reviewed changes
ui/open-api-ui/src/test/java/io/smallrye/openapi/ui/IndexCreatorTest.java Outdated
@@ -31,6 +32,8 @@ void testCreateDefault() throws IOException {
assertTrue(s.contains("<img src='logo.png' alt='SmallRye OpenAPI UI'"));
assertTrue(s.contains("dom_id: '#swagger-ui',"));
assertTrue(s.contains("deepLinking: true,"));
assertTrue(s.contains("queryConfigEnabled"));
Copy link
Contributor

Choose a reason for hiding this comment

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

If queryConfigEnabled is unset shouldn't it not be present in the resulting content ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, then it falls back to the default of swagger-ui. So if it's not set (default null) it should not be in the html. So a correct test would be that it's not there. Then when it's set to true or false, it should be there with the value

phillip-kruger
phillip-kruger requested changes Feb 2, 2023
View reviewed changes
ui/open-api-ui/src/test/java/io/smallrye/openapi/ui/IndexCreatorTest.java Outdated
@@ -31,6 +32,8 @@ void testCreateDefault() throws IOException {
assertTrue(s.contains("<img src='logo.png' alt='SmallRye OpenAPI UI'"));
assertTrue(s.contains("dom_id: '#swagger-ui',"));
assertTrue(s.contains("deepLinking: true,"));
assertTrue(s.contains("queryConfigEnabled"));
Copy link
Member

Choose a reason for hiding this comment

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

Yes, then it falls back to the default of swagger-ui. So if it's not set (default null) it should not be in the html. So a correct test would be that it's not there. Then when it's set to true or false, it should be there with the value

@mfpc @gastaldi
Added property config query-config-enabled
288a53d 
Co-Authored-By: George Gastaldi <gegastaldi@gmail.com>
@mfpc mfpc force-pushed the query-config-enabled branch from 0cecc0d to 288a53d Compare February 6, 2023 12:13
@mfpc
Copy link
Contributor Author

mfpc commented Feb 6, 2023

All adjust done.

@phillip-kruger
Copy link
Member

LGTM. Thanks

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phillip-kruger
Copy link
Member

@MikeEdgar if you are happy you can merge :)

@MikeEdgar MikeEdgar added this to the 3.2.0 milestone Feb 7, 2023
@MikeEdgar MikeEdgar merged commit 2cd8741 into smallrye:main Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phillip-kruger phillip-kruger phillip-kruger approved these changes

@gastaldi gastaldi gastaldi left review comments

@MikeEdgar MikeEdgar MikeEdgar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mfpc @phillip-kruger @gastaldi @MikeEdgar