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

SONARPY-2331 Implement setting metaclassFQN field for ClassType to ClassDescriptor conversion #2153

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

maksim-grebeniuk-sonarsource
Copy link
Contributor

@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource commented Nov 13, 2024

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!
Left a few comments, but they're quite open to discussion - not sure we need to make changes as part of this PR.

return new ClassDescriptor(symbolName, symbolFqn,
superClasses,
memberDescriptors,
type.hasDecorators(),
type.definitionLocation().orElse(null),
hasSuperClassWithoutDescriptor,
type.hasMetaClass(),
null,
metaclassFQN,

Choose a reason for hiding this comment

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

Not strictly related to the PR, but I see the metaclassFQN method is not annotated as CheckForNull which is probably misleading.

Also, I'm a bit concerned that we're going to cement the usage of the metaclassFQN somewhat inconsistently (because importType.importPath() does not produce a fully qualified name according to our definition). I think it's okay for now, but we should keep in mind that this probably need to change eventually (like for decorators).

@@ -1125,6 +1125,18 @@ class Field(MetaField): ...
assertThat(symbol.hasUnresolvedTypeHierarchy()).isTrue();
}

@Test
void class_wth_metaclass() {

Choose a reason for hiding this comment

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

It would probably make sense to have some other tests where the metaclass is local or unresolved. I know it's basically testing typeFqn again, but since it's not a fully valid implementation (cf above comment), we probably want to have it documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@guillaume-dequenne-sonarsource guillaume-dequenne-sonarsource merged commit da5face into master Nov 14, 2024
11 checks 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