Skip to content

Conversation

@bitxon
Copy link
Contributor

@bitxon bitxon commented May 6, 2023

No description provided.

@bitxon bitxon requested a review from oleg-nenashev as a code owner May 6, 2023 14:48
@bitxon bitxon force-pushed the feature/align-method-names branch from 92bbcb9 to 1e4d0fb Compare May 6, 2023 14:50
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

baseUrl and url are actually not aligned with the common Java notation. I am not strongly against that, but maybe we could keep the compliant methods too and later maybe have them in WireMock also

@bitxon
Copy link
Contributor Author

bitxon commented May 8, 2023

baseUrl and url are actually not aligned with the common Java notation. I am not strongly against that, but maybe we could keep the compliant methods too and later maybe have them in WireMock also

@oleg-nenashev What is preferred option

Option A: to have both methods like

baseUrl(); // same as in WireMockServer
getBaseUrl(); // classic getter method 
url(String);
getUrl(String);
port();
getPort();

Option B: just add suffix get

getBaseUrl();
getUrl(String);
getPort();

Personally i like option B, but then methods will be not aligned with WireMockServer class

@bitxon bitxon force-pushed the feature/align-method-names branch from 1e4d0fb to 63b843a Compare May 10, 2023 18:57
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Apart from the NPE risk, it is generally okay. To address the actual use-case of following Java best practices for WiremockServer, I would generally suggest a patch in Wiremock within the scope of 3.0. At the same time, there are plenty of such APIs and maybe talking to @tomakehurst first makes sense.

Happy to approve once the NPE thing is addressed. Will enable SpotBugs too later

@oleg-nenashev oleg-nenashev added breaking This change breaks backward compatibility enhancement New feature or request labels May 13, 2023
@bitxon bitxon force-pushed the feature/align-method-names branch from 63b843a to 93007e9 Compare May 13, 2023 16:05
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

BTW no need to force-push each time, we can always squash-merge if needed

@oleg-nenashev oleg-nenashev merged commit ebb9b7a into wiremock:main May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking This change breaks backward compatibility enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants