-
Notifications
You must be signed in to change notification settings - Fork 91
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
Implement a Matcher interface to manage ArbitraryBuilders with a single variable #1062
Implement a Matcher interface to manage ArbitraryBuilders with a single variable #1062
Conversation
@seongahjo 이번 구현은
이때 초기화 하는 위치는
저번 PR에서 성아님께서 만약 더 좋은 아이디어가 있으시다면 공유 부탁드립니다🙂 |
|
||
public final class NamedMatcherOperator<T> extends MatcherOperator<T> { | ||
private final String registeredName; | ||
private boolean active; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상태를 가지지 않는 게 좋을 것 같습니다 즉 변경되는 값이 없는 방향이 좋을 것 같습니다.
active
같은 상태를 가지고 상태를 외부에서 제어하는 방향을 피하는 게 좋을 것 같습니다.
이렇게 구현하면 NamedMatcherOperator
는 matcher의 판단을 외부로 넘겨주고 있으므로 matcher 인터페이스를 구현한다고 보기 어렵습니다.
지금 구현은 간단하지만 복잡해질수록 코드만 보고 동작을 예측하기 어려워집니다.
상태마다 경우의 수가 나뉘기 때문에 런타임에만 동작을 확인할 수 있는 블랙박스가 될 가능성이 높습니다.
registeredName
을 노출하지 않고 selectName
을 파라미터로 입력받는 메소드를 추가하는 방향이 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 구현은 간단하지만 복잡해질수록 코드만 보고 동작을 예측하기 어려워집니다.
상태마다 경우의 수가 나뉘기 때문에 런타임에만 동작을 확인할 수 있는 블랙박스가 될 가능성이 높습니다.
동의합니다.
이러한 관점으로 바라보는 것은 생각하지 못 했는데 성아님 덕분에 새로운 시각을 얻게 되었습니다.🙂
registeredName을 노출하지 않고 selectName을 파라미터로 입력받는 메소드를 추가하는 방향이 좋을 것 같습니다.
말씀해주신대로 selectName
을 파라미터로 입력 받는 메소드를 추가했습니다.
의도한대로 selectName
에서 선택한 register
연산은 잘 저장이 되나, 한 가지 사이드 이펙트가 발생했습니다.
하기의 테스트 코드와 같이 selectName
에서 register
연산이 선택되지 않았는데도 적용이 됩니다.
MatcherOperator
을 확장한 클래스에서 별도의 처리를 통해 선택되지 않은 register
연산은 적용이 안 되게 하려고 시도했으나, 설계를 하는데에 어려움을 겪고 있습니다.
Line 807 in e18495e
@Property |
- 혹시 제가 성아님의 코멘트를 잘못 이해하여 잘못 구현된 부분이 있다면 코멘트 부탁드립니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하기의 테스트 코드와 같이 selectName에서 register 연산이 선택되지 않았는데도 적용이 됩니다.
MatcherOperator을 확장한 클래스에서 별도의 처리를 통해 선택되지 않은 register 연산은 적용이 안 되게 하려고 시도했으나, 설계를 하는데에 어려움을 겪고 있습니다.
이건 registerName와 다르게 register 연산들은 Matcher 인터페이스로 match 여부를 확인해서 생긴 side-effect로 보입니다.
즉, 아래처럼 분기가 되고 있는 것입니다.
- register 연산 -> Matcher로 비교
- registerName 연산 -> NamedMatcherOperator#matchRegisteredName로 비교
이 문제를 해결하기 위해서는 연산 적용 여부를 동일한 흐름으로 확인할 수 있게 수정해야 합니다.
조금 더 구체적으로 이야기하면 연산 적용 여부를 확인하는 Matcher 인터페이스에 property 외에 메타 데이터로 비교하는 메소드를 추가하고 MatcherOperator에서 해당 메소드를 사용하도록 수정하면 될 것 같습니다.
지금 생각나는 방안은 Matcher 인터페이스에 메타데이터로 비교하는 default 메소드를 추가하고 NamedMatcherOperator
에서 default 메소드를 확장하는 방향은 어떨까 싶습니다. 그리고 MatcherOperator 에서만 Matcher#match(Property, MatcherMetadata)
을 사용하게 변경하면 될 것 같습니다.
public interface Matcher {
boolean match(Property property)
default boolean match(Property property, @Nullable MatcherMetadata metadata){
return this.match(property);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
성아님의 말씀처럼 연산 적용 여부를 동일한 흐름으로 확인할 수 있게 수정하였습니다.
NamedMatcherOperator
에서 default 메서드를 확장하고,MatcherOperator
에서는 (property
,MatcherMetadata
)를 통해 적용 여부를 확인했습니다.
MatcherMetadata
는 추후에 하기와 같이 확장 계획이 있기 때문에 String
외 다른 타입도 처리할 수 있게 설계했습니다.
- (name, 타입)을 키로 register할 수 있게 구현
다만, 제가 구현한 방향에서 신경 쓰이는 부분이 하나 있습니다.
MatcherOperator
에 selectName
필드를 추가하면서 코드가 다소 복잡해진 것 같습니다.
selectName
필드는 특정 연산(selectName
을 통해 선택된 register
연산)에서만 적용되고, 그 외에는 null
이 됩니다.
즉, 이 필드의 값이 런타임에서 결정되므로, 코드가 블랙박스와 같이 동작할 수 있다는 우려가 있습니다.
그러나 MatcherOperator
가 selectName
을 저장하지 않으면, 다른 연산에서도 선택된 name
정보를 계속 유지해야 합니다. 이를 위해 매번 파라미터로 name
을 주고받는 것은 바람직한 설계가 아니라고 판단하여, 결국 MatcherOperator
에 selectName
필드를 추가하기로 결정했습니다.
하기의 그림과 같이 selectName
시점에서 선택된 name
을 반영합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectName
이나 matchRegisteredName
대신 default boolean match(Property property, @Nullable MatcherMetadata metadata)
를 사용하면 될 것 같은데요 두 메서드가 어떤 이유로 필요한걸까요??
그러나 MatcherOperator가 selectName을 저장하지 않으면, 다른 연산에서도 선택된 name 정보를 계속 유지해야 합니다. 이를 위해 매번 파라미터로 name을 주고받는 것은 바람직한 설계가 아니라고 판단하여,
저장을 해야하고, 선택된 name 정보를 유지해야하는 이유가 뭔가요?? 이해를 못했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저장을 해야하고, 선택된 name 정보를 유지해야하는 이유가 뭔가요?? 이해를 못했습니다.
selectName
에서만 Matchermetadata
를 사용한다면 selectName
을 MatcherOperator
에 저장할 필요가 없으나 SimpleObject
와 같이 재귀를 통해 값을 생성하는 경우 selectName
을 Matchermetadata
에 저장하지 않으면 선택된 name
정보를 모른다고 생각했습니다. 그로 인해 저장을 해야하고, 선택된 name
을 유지해야 한다고 작성했던 것입니다!
- 재귀를 할 때
Matcher#match(property)
를 통해 적용 여부를 결정하면 선택되지 않은register
연산도 적용이 되어, 동일하게match(Property, Matchermetadata)
로 연산 적용 여부를 판단해야 한다고 생각했습니다! 그리고 이때MatcherMetadata
를 생성하려면selectName
이 필요하기에 저장을 했습니다!
selectName
이나 matchRegisteredName
메서드도 위의 이유로 메서드를 추가했습니다!
제가 설계의 방향을 잘못 잡았다면 지적 부탁드립니다 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimpleObject와 같이 재귀를 통해 값을 생성하는 경우 selectName을 Matchermetadata에 저장하지 않으면 선택된 name 정보를 모른다고 생각했습니다. 그로 인해 저장을 해야하고, 선택된 name을 유지해야 한다고 작성했던 것입니다!
앗 넵 이해했습니다. 자세한 설명 감사합니다!
제 생각에는 MatcherOperator에 유지하는 것보다 ArbitraryBuilderContext에 두면 어떨까 싶습니다.
MatcherOperator에 두면 Matcher의 관심사가 외부로 유출되는 문제가 있는 것 같습니다.
ArbitraryBuilder마다 registerName이 달라질 수 있으므로 ArbitraryBuilderContext에 두고 꺼내서 써도 될 것 같다는 생각이 드네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatcherOperator에 두면 Matcher의 관심사가 외부로 유출되는 문제가 있는 것 같습니다.
좋은 생각인 것 같습니다👍🏻
말씀해주신대로 ArbitraryBuilderContext
에 selectNames
를 저장한 뒤, register 연산을 적용할 때마다 꺼내서 사용하는 방식으로 구현했습니다!
default boolean match(Property property, @Nullable MatcherMetadata<?> matcherMetadata) { | ||
return match(property); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메서드를 사용할 때는 matcherMetadata
가 null이 아닐 때를 가정하므로 @Nullable
마킹이 없어도 될 것 같습니다.
아래 Condition 인터페이스와 유사한 방향이라고 보시면 될 것 같습니다.
https://github.com/spring-projects/spring-framework/blob/main/spring-context/src/main/java/org/springframework/context/annotation/Condition.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
해당 어노테이션 삭제하도록 하겠습니다.
참고 자료 감사합니다👍🏻
name, new MatcherOperator<>(matcherOperator.getMatcher(), matcherOperator.getOperator().apply(this)) | ||
mapsByRegisteredName.forEach((registeredName, matcherOperator) -> { | ||
registeredArbitraryBuilders.add( | ||
NamedMatcherOperatorAdapter.adapt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NamedMatcherOperatorAdapter
가 없어도 될 것 같은데 어떤 이유로 추가하신 걸까요???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음 설계를 할 때 추가하고 삭제를 하지 않았네요,,
어댑터를 사용하지 않고 직접 추가하는 방식으로 수정하겠습니다.
if (builderContext.getSelectedNames().isEmpty()) { | ||
return it.match(property, NamedMatcherMetadata::empty); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it.match(property, NamedMatcherMetadata::empty
대신 it.match(property)
로 사용해도 되지 않나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it.match(property, NamedMatcherMetadata::empty대신 it.match(property)로 사용해도 되지 않나요??
이렇게 진행을 하면 registeredName API를 통해 register 연산을 등록한 후, selectName을 호출하지 않았는데도 적용이 되는 사이드 이펙트가 발생할 것 같습니다.
실제로 실행을 시켜본 결과 하기와 같은 테스트 코드가 성공함을 확인했습니다.
registeredName API를 초기 설계할 때 선택된 이름만 적용이 되도록 설계를 해서 위와 같은 경우는 적용이 되면 안 되는 경우라고 생각합니다.
@Property
void registeredNameWithNoSelectName() {
FixtureMonkey sut = FixtureMonkey.builder()
.registeredName(
"test",
String.class,
monkey -> monkey.giveMeBuilder("test")
)
.build();
SimpleObject actual = sut.giveMeBuilder(SimpleObject.class)
// .selectName("test")
.sample();
then(actual.getStr()).isEqualTo("test");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registeredName API를 초기 설계할 때 선택된 이름만 적용이 되도록 설계를 해서 위와 같은 경우는 적용이 되면 안 되는 경우라고 생각합니다.
registeredName도 register인 만큼 랜덤하게 선택된다고 생각했는데 아니었군요.
registeredName가 selectName으로만 동작한다면 너무 제한적이라고 생각하는데 어떻게 생각하시나요?? 어떤 시나리오에서 registeredName을 사용할지 아직은 잘 모르겠습니다. 말씀하신 초기 설계에서 registeredName와 InnerSpec이 어떤 게 다른지 잘 모르겠습니다.
이해하기 쉬운 주문 도메인을 구현한다고 가정했을 때, 제가 생각하는 시나리오는 아래와 같습니다.
옵션
registerdName("배송이 있는 주문", ...);
registerdName("배송이 없는 주문", ...);
주문을 생성할 때마다 위에 두 register로 등록한 연산 셋 중 하나를 선택해서 생성합니다.
테스트 케이스에서 배송이 있는 주문만 필요할 경우 selectName으로 선택합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고민해보니, 성아님의 말씀대로 현재 설계가 InnerSpec과 크게 다르지 않은 것 같습니다.
초기에는 선택이라는 요소에 집중하여 설계를 진행하다 보니, 제한적인 구조가 된 것 같습니다.😅
성아님의 의견에 전적으로 동의를 하는데 한 가지 궁금한 점이 있습니다.
registeredName도 register인 만큼 랜덤하게 선택된다고 생각했는데 아니었군요.
현재 registeredName은 selectName이 없는 경우 가장 먼저 호출된 registeredName이 적용되는 구조입니다.
성아님께서 registeredName
도 랜덤하게 선택된다고 하셨는데 조금만 추가 설명 부탁드립니다..!
- 어떤 것을 랜덤하게 선택하려고 의도하셨는지 궁금합니다! (랜덤적인 요소가 들어가면 저도 좋을 것 같다고 생각합니다!!)
그리고 제 개인적인 생각인데 JUnit의 order
와 같이 register 연산을 등록할 때 순서
를 부여하는 것도 괜찮을 것 같습니다.
현재 registeredName은 selectName이 없는 경우 가장 먼저 호출된 registeredName이 적용되는 구조입니다. (동일한 클래스 인 경우)
코드와 함께 추가 설명을 하자면, 하기의 코드와 같이 String이라는 클래스에 대해 register 연산이 여러 개 등록된 경우 가장 먼저 등록된 배송이 있는 주문이 선택 됩니다.
@Property
void registeredNameRandomly() {
FixtureMonkey sut = FixtureMonkey.builder()
.registeredName(
"배송이 있는 주문",
String.class,
monkey -> monkey.giveMeBuilder("exists")
)
.registeredName(
"배송이 없는 주문",
String.class,
monkey -> monkey.giveMeBuilder("not exist")
)
.registeredName(
"배송이 취소된 주문",
String.class,
monkey -> monkey.giveMeBuilder("cancelled")
)
.registeredName(
"배송이 연기된 주문",
Integer.class,
monkey -> monkey.giveMeBuilder(20241102)
)
.build();
SimpleObject actual = sut.giveMeBuilder(SimpleObject.class)
.sample();
then(actual.getStr()).isEqualTo("exists");
then(actual.getWrapperInteger()).isEqualTo(20241102);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
성아님께서 registeredName도 랜덤하게 선택된다고 하셨는데 조금만 추가 설명 부탁드립니다..!
코드와 함께 추가 설명을 하자면, 하기의 코드와 같이 String이라는 클래스에 대해 register 연산이 여러 개 등록된 경우 가장 먼저 등록된 배송이 있는 주문이 선택 됩니다.
`첫 번째로 등록한 연산을 적용하는 것보다 적용할 regiserGroup을 임의로 (arbitrary) 정하는 방향을 생각했습니다.
name을 지정하지 않으면 지정한 그룹 중 임의의 그룹이 와도 테스트를 통과해야 한다고 생각했습니다.
만약 통과하지 못한다면 다음 두 가지 경우일 것이라고 생각합니다.
registerGroup
가 해당 객체가 가져야 하는 기본 형태를 나타내지 못했다. ex. 매우 지엽적인 케이스를 registerGroup으로 설정함.- 로직이 일관성을 제공하지 못했으므로 생각하지 못한 엣지 케이스가 있을 수 있다.
그리고 제 개인적인 생각인데 JUnit의 order와 같이 register 연산을 등록할 때 순서를 부여하는 것도 괜찮을 것 같습니다.
우선순위가 있는 것도 좋을 것 같은데, 다음 PR에서 고민해보면 어떨까 싶네요.
어떻게 입력받을지 고민해보면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
첫 번째로 등록한 연산을 적용하는 것보다 적용할 regiserGroup을 임의로 (arbitrary) 정하는 방향을 생각했습니다.
이번 PR의 목적은 ArbitraryBuilder를 관리하는 부분을 리팩토링하는 것이므로, 이번 PR과는 내용이 어울리지 않다고 생각합니다!
그래서 우선순위 기능과 같이 다음 PR에서 진행해도 괜찮을까요?
우선순위가 있는 것도 좋을 것 같은데, 다음 PR에서 고민해보면 어떨까 싶네요.
어떻게 입력받을지 고민해보면 좋을 것 같습니다.
넵! 성아님께서 괜찮으시다면 연산을 임의로 선택하는 것과 함께 고민해보겠습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 다음 PR에서 진행하시죠 👍
|
||
import com.navercorp.fixturemonkey.api.property.Property; | ||
|
||
public final class NamedMatcherOperator<T> extends MatcherOperator<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatcherOperator에서 Matcher 인터페이스를 구현하고 있지만 해당 기능을 필드인 Matcher
에게 위임하고 있습니다.
NamedMatcherOperator
는 Matcher 역할을 하고 있는데, 이런 방향성보다는 Matcher를 활용하는 게 더 좋을 것 같습니다.
NamedMatcher
같은 걸 만들고 MatcherOperator는 그대로 유지하는 방향이 좋을 것 같습니다.
MatcherOperator
에서 default method인 match를 위임하는 코드도 추가해야할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다!
해당 방식으로 작업 하겠습니다.
@@ -24,7 +24,7 @@ | |||
import com.navercorp.fixturemonkey.api.property.Property; | |||
|
|||
@API(since = "0.4.0", status = Status.MAINTAINED) | |||
public final class MatcherOperator<T> implements Matcher { | |||
public class MatcherOperator<T> implements Matcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatcherOperator는 final class로 유지하여 상속을 안하는 방향이 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
원상 복구 시키겠습니다.
@@ -94,7 +93,7 @@ public <T> ArbitraryBuilder<T> giveMeBuilder(TypeReference<T> type) { | |||
RootProperty rootProperty = new RootProperty(type.getAnnotatedType()); | |||
|
|||
ArbitraryBuilderContext builderContext = registeredArbitraryBuilders.stream() | |||
.filter(it -> it.match(rootProperty)) | |||
.filter(it -> it.match(rootProperty, NamedMatcherMetadata::empty)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it.match(rootProperty)
을 그대로 사용해도 될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
해당 부분의 경우 property 비교를 해도 문제가 없는 것을 확인했습니다.
수정하도록 하겠습니다.
public interface MatcherMetadata<T> { | ||
T getMetadataInfo(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제네릭을 사용하는 것보다 스프링에 있는 AnnotatedTypeMetadata
처럼 구현하는 방향이 어떨까 싶습니다.
당장 필요한 정보는 name이므로 String getName()
메서드를 추가하는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
추후에 필요한 정보가 더 생기면 확장성을 고려해서 리팩토링 하겠습니다 :)
@@ -82,6 +86,7 @@ public ArbitraryBuilderContext copy() { | |||
return new ArbitraryBuilderContext( | |||
new ArrayList<>(this.manipulators), | |||
copiedContainerInfoManipulators, | |||
this.selectNames, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new ArrayList<>(this.selectNames)
로 새로운 리스트를 할당하게 하는 게 좋을 것 같습니다.
selectNames의 변경이 copy()한 인스턴스까지 전파되지 않게 하는 게 좋을 것 같습니다.
PR이 좀 길어지고 있긴 한데, 같이 논의하면서 기능이 좋은 방향으로 발전을 해나가는 것 같습니다. 👍 |
package com.navercorp.fixturemonkey.api.matcher; | ||
|
||
public interface MatcherMetadata { | ||
String getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제네릭해서 의미가 혼용될 수 있는 name
보다는 좀 더 범위가 좁은 이름을 사용하는 게 좋을 것 같습니다.
적절한 이름이 당장은 안떠오르네요; 떠오르면 코멘트로 남겨놓겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 고민을 해봤는데 getMatcherAlias
은 어떠신가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 고민하는 케이스에서는 alias
가 적합한 것 같은데, 다른 경우에는 적합하지 않겠다는 생각이 드네요.
일단 냅두고 다른 PR에서 같이 고민해보시죠. 다른 라이브러리나 프레임워크을 확인해볼 필요가 있을 것 같습니다.
public boolean matchRegisteredName(String registeredName) { | ||
return this.registeredName.equals(registeredName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 없어져도 될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 삭제하도록 하겠습니다.
|
||
package com.navercorp.fixturemonkey.api.matcher; | ||
|
||
public class NamedMatcherMetadata implements MatcherMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final로 상속을 막는 게 좋을 것 같습니다.
return it.match(property); | ||
} | ||
return builderContext.getSelectedNames().stream().anyMatch( | ||
name -> it.match(property, new NamedMatcherMetadata(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가능하면 it.match(property, metadata) 만 사용하도록 총일하는 것도 좋아보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
성아님께서 의도하신 코드가 하기와 같다면 해당 코멘트에서 얘기를 나눈 것 같습니다!
if (builderContext.getSelectedNames().isEmpty()) {
return it.match(property, new NamedMatcherMetadata(null));
}
return builderContext.getSelectedNames().stream().anyMatch(
name -> it.match(property, new NamedMatcherMetadata(name))
);
선택된 register 연산만 적용하지 않고 그 외 연산을 적용하기 위해선 it.match(property)
로 해야 할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 정리만 마무리 하고 다음 PR 작업을 해도 좋을 것 같습니다!
코드 정리 완료했습니다. 확인 부탁드립니다! 감사합니다. |
d7f8333
to
a5136b3
Compare
@@ -0,0 +1,23 @@ | |||
package com.navercorp.fixturemonkey.api.matcher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
라이센스를 추가하면 좋을 것 같습니다.
/*
* Fixture Monkey
*
* Copyright (c) 2021-present NAVER Corp.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 수정하겠습니다.
package com.navercorp.fixturemonkey.api.matcher; | ||
|
||
public final class NamedMatcherMetadata implements MatcherMetadata { | ||
private final String selectedName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
으로 일단 변경해두는 게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 우선 name으로 변경한 뒤 다른 PR에서 네이밍 고민해보겠습니다.
Summary
(Optional): Description
registeredName
String field to store and validate.metadata
that stores the name information selected by the user.How Has This Been Tested?
Is the Document updated?
No needed