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

Improve the dependency path exception message #11405

Merged
merged 4 commits into from
Dec 9, 2024

Conversation

andriy-dmytruk
Copy link
Contributor

@andriy-dmytruk andriy-dmytruk commented Dec 3, 2024

This improves the readability of message for long paths, large class names or classes with many parameters, as each class from the path is immediately visible. I changed the tests to use longer class names, which is more realistic.

The type names are also in fully-qualified shortened form, which makes them clickable in IDEs.

As a comparison this message

Message: No bean of type [io.micronaut.inject.failures.NestedDependencyFailureSpec$MyClassD] exists. 
| Path Taken: new MyClassB() --> MyClassB.propA --> new MyClassA([MyClassC propC]) --> new MyClassC([MyClassD propD])

will change to

Message: No bean of type [io.micronaut.inject.failures.NestedDependencyFailureSpec$MyClassD] exists.
Path Taken:
new i.m.i.f.N$MyClassB()
\---> i.m.i.f.N$MyClassB#propA
      \---> new i.m.i.f.N$MyClassA([MyClassC propC])
            \---> new i.m.i.f.N$MyClassC([MyClassD propD])

Part of the change is similar to visualization change for circular dependencies: #11299.

@sdelamo
Copy link
Contributor

sdelamo commented Dec 4, 2024

maybe this should target 4.8.x

@dstepanov
Copy link
Contributor

Consider adding a full class name. I think IntelliJ will provide a link to the class

@dstepanov
Copy link
Contributor

How does it look with a factory method / field?

Message: No bean of type [io.micronaut.inject.failures.NestedDependencyFailureSpec\$MyClassD] exists.$space
Path Taken:$space
new MyClassB()
\\---> MyClassB.propA
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an example of field here

@andriy-dmytruk
Copy link
Contributor Author

andriy-dmytruk commented Dec 4, 2024

How does it look with a factory method / field?

For factory there is a circular dependency test:

Message: Circular dependency detected
Path Taken:
new ElectricalGrid(List<ElectricStation E> stations)
\\---> new ElectricalGrid([List<ElectricStation E> stations])
^ \\---> ElectricStationFactory.nuclearStation([MeasuringEquipment equipment])
| \\---> MeasuringEquipment.grid
| |
+--------------+'''

@graemerocher
Copy link
Contributor

Consider adding a full class name. I think IntelliJ will provide a link to the class

this is a good point, with the new lines maybe we can include qualified names. Can you check this link works in IntelliJ?

@andriy-dmytruk
Copy link
Contributor Author

Yes, the classnames are clickable:
image

This is more readable, though:
image

I think both are good. Wdyt?

@graemerocher
Copy link
Contributor

@dstepanov WDYT?

@dstepanov
Copy link
Contributor

Intellij supports even shortened names, so we can reduce it if it's too large:

try:

    public static void main(String[] args) {
        System.out.println("io.m.r.Micronaut!");
    }

@graemerocher
Copy link
Contributor

@andriy-dmytruk can you try that?

@andriy-dmytruk
Copy link
Contributor Author

andriy-dmytruk commented Dec 4, 2024

Yes, this is actually amazing! Now there are actually 2 options:

  1. Shorten type owner names:
    image
    When clicked:
    image

  2. Do not shorten:
    image

I would say the first one is the overall best.

@dstepanov
Copy link
Contributor

Maybe we can have something like 200-300 characters per name and start reducing when it's more.

@graemerocher
Copy link
Contributor

the first one is the best yes

…t on a new line.

This improves the readability of message for long paths, large classnames or classes with many parameters, as each class from the path is immediately visible. I changed the tests to use longer classnames, which is more realistic.
Fully-qualified names are clickable in IDE terminals, which improves usability. Shortened form prevents the messages from being too convoluted.
@andriy-dmytruk andriy-dmytruk force-pushed the andriy/dependency-path-improve branch from 351a91a to c5b8ae4 Compare December 5, 2024 15:11
@andriy-dmytruk andriy-dmytruk changed the base branch from 4.7.x to 4.8.x December 5, 2024 15:11
@andriy-dmytruk
Copy link
Contributor Author

I went for using the first form always, as it is shortest, but is still clickable. We can also extend it at a later point if we experience issues with it.

Maybe we can have something like 200-300 characters per name and start reducing when it's more.

We can start a GitHub discussion, but I think shortest should be good in most cases. The chances of conflict are pretty low. If we do need this, the typical line limits are 80, 100, 120, which would mean around 60 chars limit for the name alone.

@andriy-dmytruk andriy-dmytruk changed the title Improve the dependency path exception message by printing each segment on a new line Improve the dependency path exception message Dec 5, 2024
@graemerocher graemerocher added the type: improvement A minor improvement to an existing feature label Dec 5, 2024
Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

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

just two small tweaks before merging.

core/src/main/java/io/micronaut/core/naming/NameUtils.java Outdated Show resolved Hide resolved
*/
@Experimental
public static String getShortenedName(String typeName) {
int nameStart = typeName.lastIndexOf('$');
Copy link
Contributor

Choose a reason for hiding this comment

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

check type name doesn't == null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not checked in other methods, so I assume it is not typical for typeName to be null. Added @NonNull though.

@andriy-dmytruk
Copy link
Contributor Author

@yawkat I don't understand the issue. Can you take a look?

https://ge.micronaut.io/s/szoo56vrxva66
44C5523F-E220-4E09-B7D3-AB5A84F2010C

@yawkat
Copy link
Member

yawkat commented Dec 6, 2024

that test is just flakey. i dont know why

Copy link

sonarcloud bot commented Dec 6, 2024

@graemerocher graemerocher merged commit 39cea72 into 4.8.x Dec 9, 2024
22 checks passed
@graemerocher graemerocher deleted the andriy/dependency-path-improve branch December 9, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants