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

Implement a Matcher interface to manage ArbitraryBuilders with a single variable #1062

Merged

Conversation

YongGoose
Copy link

@YongGoose YongGoose commented Oct 16, 2024

Summary

  • Implement a Matcher interface to manage ArbitraryBuilders with a single variable

(Optional): Description

How Has This Been Tested?

  • existing tests

Is the Document updated?

No needed

@YongGoose
Copy link
Author

@seongahjo
성아님! 한 가지 상의해볼 내용이 있어 코멘트 남깁니다.

이번 구현은 MatcherOperator를 상속받은 새로운 클래스를 통해 구현했습니다.
선택 여부를 관리하기 위해 active라는 boolean 필드를 추가했으며, 동작 방식은 하기와 같습니다.

  • giveMeBuilder 메서드에서 active 필드를 false로 초기화한 후, selectName 메서드에서 입력된 이름과 비교를 수행합니다.
  • 이름이 일치할 경우 active 필드를 true로 변경하여, 사용자의 입력에 따라 선택 상태가 동적으로 관리되도록 했습니다.
  • match 메서드에서 상위 클래스의 matchactive 여부를 통해 식별 합니다.
return active && super.match(property);

이때 초기화 하는 위치는 selectName의 여부와 상관 없이 초기화를 하기 위해 giveMeBulider에서 초기화를 했습니다.

selectName API를 호출하지 않는 경우, giveMeOne을 통해 생성하는 경우등 RegisteredName을 통해 등록한 register 연산이 다음 객체 생성에 영향을 미치지 않도록 했습니다.


저번 PR에서 성아님께서 (name, Property)로 식별을 하라고 하셨는데 (active, property)를 통한 식별 방식이 괜찮은지 성아님의 의견이 궁금합니다. 그리고 초기화 하는 위치도 한 번 검토 부탁드립니다!

만약 더 좋은 아이디어가 있으시다면 공유 부탁드립니다🙂


public final class NamedMatcherOperator<T> extends MatcherOperator<T> {
private final String registeredName;
private boolean active;
Copy link
Contributor

@seongahjo seongahjo Oct 17, 2024

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을 파라미터로 입력받는 메소드를 추가하는 방향이 좋을 것 같습니다.

Copy link
Author

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 연산은 적용이 안 되게 하려고 시도했으나, 설계를 하는데에 어려움을 겪고 있습니다.

  • 혹시 제가 성아님의 코멘트를 잘못 이해하여 잘못 구현된 부분이 있다면 코멘트 부탁드립니다..!

Copy link
Contributor

@seongahjo seongahjo Oct 20, 2024

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);
        }
}

Copy link
Author

@YongGoose YongGoose Oct 22, 2024

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할 수 있게 구현

다만, 제가 구현한 방향에서 신경 쓰이는 부분이 하나 있습니다.
MatcherOperatorselectName 필드를 추가하면서 코드가 다소 복잡해진 것 같습니다.

selectName 필드는 특정 연산(selectName을 통해 선택된 register 연산)에서만 적용되고, 그 외에는 null이 됩니다.
즉, 이 필드의 값이 런타임에서 결정되므로, 코드가 블랙박스와 같이 동작할 수 있다는 우려가 있습니다.

그러나 MatcherOperatorselectName을 저장하지 않으면, 다른 연산에서도 선택된 name 정보를 계속 유지해야 합니다. 이를 위해 매번 파라미터로 name을 주고받는 것은 바람직한 설계가 아니라고 판단하여, 결국 MatcherOperatorselectName 필드를 추가하기로 결정했습니다.

하기의 그림과 같이 selectName 시점에서 선택된 name을 반영합니다.

image

Copy link
Contributor

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 정보를 유지해야하는 이유가 뭔가요?? 이해를 못했습니다.

Copy link
Author

Choose a reason for hiding this comment

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

저장을 해야하고, 선택된 name 정보를 유지해야하는 이유가 뭔가요?? 이해를 못했습니다.

selectName에서만 Matchermetadata를 사용한다면 selectNameMatcherOperator에 저장할 필요가 없으나 SimpleObject와 같이 재귀를 통해 값을 생성하는 경우 selectNameMatchermetadata에 저장하지 않으면 선택된 name 정보를 모른다고 생각했습니다. 그로 인해 저장을 해야하고, 선택된 name을 유지해야 한다고 작성했던 것입니다!

  • 재귀를 할 때 Matcher#match(property)를 통해 적용 여부를 결정하면 선택되지 않은 register 연산도 적용이 되어, 동일하게 match(Property, Matchermetadata)로 연산 적용 여부를 판단해야 한다고 생각했습니다! 그리고 이때 MatcherMetadata를 생성하려면 selectName이 필요하기에 저장을 했습니다!

selectName이나 matchRegisteredName 메서드도 위의 이유로 메서드를 추가했습니다!

제가 설계의 방향을 잘못 잡았다면 지적 부탁드립니다 🙂

Copy link
Contributor

@seongahjo seongahjo Oct 29, 2024

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에 두고 꺼내서 써도 될 것 같다는 생각이 드네요.

Copy link
Author

Choose a reason for hiding this comment

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

MatcherOperator에 두면 Matcher의 관심사가 외부로 유출되는 문제가 있는 것 같습니다.

좋은 생각인 것 같습니다👍🏻

말씀해주신대로 ArbitraryBuilderContextselectNames를 저장한 뒤, register 연산을 적용할 때마다 꺼내서 사용하는 방식으로 구현했습니다!

Comment on lines 33 to 36
default boolean match(Property property, @Nullable MatcherMetadata<?> matcherMetadata) {
return match(property);
}

Copy link
Contributor

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

Copy link
Author

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

NamedMatcherOperatorAdapter가 없어도 될 것 같은데 어떤 이유로 추가하신 걸까요???

Copy link
Author

Choose a reason for hiding this comment

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

처음 설계를 할 때 추가하고 삭제를 하지 않았네요,,
어댑터를 사용하지 않고 직접 추가하는 방식으로 수정하겠습니다.

Comment on lines 158 to 160
if (builderContext.getSelectedNames().isEmpty()) {
return it.match(property, NamedMatcherMetadata::empty);
}
Copy link
Contributor

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)로 사용해도 되지 않나요??

Copy link
Author

@YongGoose YongGoose Nov 2, 2024

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");
}

Copy link
Contributor

@seongahjo seongahjo Nov 2, 2024

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으로 선택합니다.

Copy link
Author

@YongGoose YongGoose Nov 2, 2024

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);
}

Copy link
Contributor

@seongahjo seongahjo Nov 7, 2024

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을 지정하지 않으면 지정한 그룹 중 임의의 그룹이 와도 테스트를 통과해야 한다고 생각했습니다.

만약 통과하지 못한다면 다음 두 가지 경우일 것이라고 생각합니다.

  1. registerGroup가 해당 객체가 가져야 하는 기본 형태를 나타내지 못했다. ex. 매우 지엽적인 케이스를 registerGroup으로 설정함.
  2. 로직이 일관성을 제공하지 못했으므로 생각하지 못한 엣지 케이스가 있을 수 있다.

그리고 제 개인적인 생각인데 JUnit의 order와 같이 register 연산을 등록할 때 순서를 부여하는 것도 괜찮을 것 같습니다.
우선순위가 있는 것도 좋을 것 같은데, 다음 PR에서 고민해보면 어떨까 싶네요.
어떻게 입력받을지 고민해보면 좋을 것 같습니다.

Copy link
Author

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에서 고민해보면 어떨까 싶네요.
어떻게 입력받을지 고민해보면 좋을 것 같습니다.

넵! 성아님께서 괜찮으시다면 연산을 임의로 선택하는 것과 함께 고민해보겠습니다 :)

Copy link
Contributor

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> {
Copy link
Contributor

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를 위임하는 코드도 추가해야할 것 같습니다.

Copy link
Author

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

MatcherOperator는 final class로 유지하여 상속을 안하는 방향이 좋을 것 같습니다.

Copy link
Author

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

it.match(rootProperty)을 그대로 사용해도 될 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵!

해당 부분의 경우 property 비교를 해도 문제가 없는 것을 확인했습니다.
수정하도록 하겠습니다.

Comment on lines 21 to 23
public interface MatcherMetadata<T> {
T getMetadataInfo();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

제네릭을 사용하는 것보다 스프링에 있는 AnnotatedTypeMetadata처럼 구현하는 방향이 어떨까 싶습니다.
당장 필요한 정보는 name이므로 String getName() 메서드를 추가하는 건 어떨까요?

https://github.com/spring-projects/spring-framework/blob/a3b979c5ecb7eda96ebf264672ce522177c6fc77/spring-core/src/main/java/org/springframework/core/type/AnnotatedTypeMetadata.java#L54

Copy link
Author

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,
Copy link
Contributor

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()한 인스턴스까지 전파되지 않게 하는 게 좋을 것 같습니다.

@seongahjo
Copy link
Contributor

PR이 좀 길어지고 있긴 한데, 같이 논의하면서 기능이 좋은 방향으로 발전을 해나가는 것 같습니다. 👍

package com.navercorp.fixturemonkey.api.matcher;

public interface MatcherMetadata {
String getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

제네릭해서 의미가 혼용될 수 있는 name보다는 좀 더 범위가 좁은 이름을 사용하는 게 좋을 것 같습니다.

적절한 이름이 당장은 안떠오르네요; 떠오르면 코멘트로 남겨놓겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

저도 고민을 해봤는데 getMatcherAlias은 어떠신가요??

Copy link
Contributor

@seongahjo seongahjo Nov 8, 2024

Choose a reason for hiding this comment

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

지금 고민하는 케이스에서는 alias가 적합한 것 같은데, 다른 경우에는 적합하지 않겠다는 생각이 드네요.

일단 냅두고 다른 PR에서 같이 고민해보시죠. 다른 라이브러리나 프레임워크을 확인해볼 필요가 있을 것 같습니다.

Comment on lines 14 to 16
public boolean matchRegisteredName(String registeredName) {
return this.registeredName.equals(registeredName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 없어져도 될 것 같습니다.

Copy link
Author

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 {
Copy link
Contributor

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))
Copy link
Contributor

@seongahjo seongahjo Nov 7, 2024

Choose a reason for hiding this comment

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

가능하면 it.match(property, metadata) 만 사용하도록 총일하는 것도 좋아보입니다.

Copy link
Author

@YongGoose YongGoose Nov 7, 2024

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)로 해야 할 것 같습니다.

Copy link
Contributor

@seongahjo seongahjo left a comment

Choose a reason for hiding this comment

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

코드 정리만 마무리 하고 다음 PR 작업을 해도 좋을 것 같습니다!

@YongGoose YongGoose marked this pull request as ready for review November 8, 2024 05:45
@YongGoose
Copy link
Author

코드 정리만 마무리 하고 다음 PR 작업을 해도 좋을 것 같습니다!

코드 정리 완료했습니다. 확인 부탁드립니다!
다음 PR에서는 register 연산 임의로 정하는 것과 우선순위 기능에 대해 작업을 해보겠습니다!

감사합니다.☺️

@YongGoose YongGoose force-pushed the feature/refactor-matcher-operator branch from d7f8333 to a5136b3 Compare November 8, 2024 05:55
@YongGoose YongGoose changed the title Extend MatcherOperator to manage ArbitraryBuilders with a single variable Implement a Matcher interface to manage ArbitraryBuilders with a single variable Nov 8, 2024
@@ -0,0 +1,23 @@
package com.navercorp.fixturemonkey.api.matcher;
Copy link
Contributor

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.
 */

Copy link
Author

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

name으로 일단 변경해두는 게 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵! 우선 name으로 변경한 뒤 다른 PR에서 네이밍 고민해보겠습니다.

@seongahjo seongahjo merged commit e508ba8 into naver:31-register Nov 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants