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

Operators inner classes' Scannable#name is always "s" #2526

Closed
bsideup opened this issue Dec 3, 2020 · 4 comments
Closed

Operators inner classes' Scannable#name is always "s" #2526

bsideup opened this issue Dec 3, 2020 · 4 comments
Assignees
Labels
good first issue Ideal for a new contributor, we'll help type/bug A general bug

Comments

@bsideup
Copy link
Contributor

bsideup commented Dec 3, 2020

Reproducer:

assertThat(Scannable.from(Operators.emptySubscription()).name()).isEqualsTo("empty")
@bsideup bsideup added the type/bug A general bug label Dec 3, 2020
@simonbasle simonbasle added this to the 3.4.2 milestone Dec 3, 2020
@simonbasle simonbasle added the good first issue Ideal for a new contributor, we'll help label Dec 9, 2020
@simonbasle
Copy link
Member

It would be fairly easy to fix by overriding the stepName() method of such classes.

@ericbottard ericbottard self-assigned this Jan 5, 2021
@ericbottard
Copy link
Contributor

Apart from the obvious fix via the override, isn't weird that the current heuristic uses the enclosing class name rather than the enclosed one?

		String name = toString();
		if (name.contains("@") && name.contains("$")) {
			name = name
				.substring(0, name.indexOf('$'))
				.substring(name.lastIndexOf('.') + 1);
		}
		String stripped = OPERATOR_NAME_UNRELATED_WORDS_PATTERN
			.matcher(name)
			.replaceAll("");

		if (!stripped.isEmpty()) {
			return stripped.substring(0, 1).toLowerCase() + stripped.substring(1);
		}
		return stripped;

First of all, it does special treatment iff we're dealing with a non-overridden toString() AND it's an inner class. Why not detect non-customized toString() in all cases?

Also, InnerConsumer has a copy-paste of this code but using getClass().getSimpleName(). Thus, the presence of @ in the resulting string is unlikely to happen.

Lastly, shouldn't Subscription be part of the replacing regex?

@ericbottard
Copy link
Contributor

See also #2547

@simonbasle
Copy link
Member

I think we can remove the special case for InnerConsumer by using getClass().getName() in Scannable directly rather than toString(). We'd then accommodate for stripping package and then removing the keywords most likely found in base class. This would effectively also fix #2547.

For the s aspect, ie inner Scannable classes defined inside a util parent class like Operators, the fix is to have these explicitly define stepName with a meaningful human readable value.

This includes:

  • Scannable.Attr.NULL_SCAN
  • Scannable.Attr.UNAVAILABLE_SCAN
  • Operators.EmptySubscription
  • Operators.CancelledSubscription
  • Operators.ScalarSubscription

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for a new contributor, we'll help type/bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants