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

Simplify Extentions api - Remove overhead with id #36

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bitxon
Copy link
Contributor

@bitxon bitxon commented May 7, 2023

Motivation

  • Param id does not bring any benefits and looks redundant
  • Store all extensionJars in a LinkedHashSet to preserve an order and keep only unique values
  • Store all extensionClassNames in a LinkedHashSet to preserve an order and keep only unique value

@bitxon bitxon requested a review from oleg-nenashev as a code owner May 7, 2023 10:10
@bitxon bitxon force-pushed the feature/simplify-ext-config branch from 263feef to 570a7a7 Compare May 8, 2023 16:56
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.

Thanks for the patch, but I actually did it for future extensibility and traceabilty reasons. Classes are private for the same reasons - internal use for now. I would like to keep the source ID in the metadata, and to extend that in the future. At the same time, it would be possible to introduce new methods that would infer the IDs from the file names by default, I guess it would address DevX simplicity case

@bitxon
Copy link
Contributor Author

bitxon commented May 8, 2023

Thanks for the patch, but I actually did it for future extensibility and traceabilty reasons. Classes are private for the same reasons - internal use for now. I would like to keep the source ID in the metadata, and to extend that in the future. At the same time, it would be possible to introduce new methods that would infer the IDs from the file names by default, I guess it would address DevX simplicity case

I understand intention of this, but here is a few things that concerns me in current api

  1. it forces developers to create random string literal for each method invocation, since this id has no influence on behavior values will be very random (absence of control / validation)

     new WireMockContainer()
         .withExtention("1", ..., ...)
         .withExtention("B", ..., ...)
         .withExtention("III", ..., ...)

    If maintainer of the library decide(in some future) to change meaning or utilization of id it will be painful.

  2. Uniqueness of the extension = jarFile+className, so field id brings potential confusion

  3. (assumption) In majority of case developers use one or two extensions - so there is will not be much things to trace

My suggestion about api is the following

withExtention(String className, File jar) // 1
withExtention(Collection<String> classNames, File jar) // 2
withExtention(Collection<String> classNames, Path directoryWithJars) // 3

But if in case of 1 & 2 this is easy to calculate unique identifier = className+jarName to keep in in metadata, then in case 3 it is sophisticated

@oleg-nenashev
Copy link
Member

My suggestion about api is the following

I agree with this approach. It is one of the ways to "infer" IDs like in the previous comment
So I would add new methods that infer the IDs but would also keep the previous ones. No objections in making them package scope for now, but it would also mean a breaking change. No problem but maybe better to keep them

@oleg-nenashev
Copy link
Member

FTR my Slack comment:

Indeed it might be over-engineering at this stage. I am just looking at wiremock/wiremock#1512 which @tomakehurst wants to see soon. It would be basically introducing a much more sophisticated engine, maybe with some notion for plugins and extension IDs like in Jenkins. So I put a very basic stub for the future, but indeed it might be conffusing as a standalone PR

@oleg-nenashev oleg-nenashev added the enhancement New feature or request label May 13, 2023
@bitxon bitxon force-pushed the feature/simplify-ext-config branch from 570a7a7 to 1104494 Compare August 19, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants