Fix name resolving issues #663
Conversation
…e generation into a function to be able to test it.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #663 +/- ##
==========================================
+ Coverage 92.45% 92.49% +0.04%
==========================================
Files 47 47
Lines 8056 8103 +47
Branches 1925 1935 +10
==========================================
+ Hits 7448 7495 +47
Misses 354 354
Partials 254 254
☔ View full report in Codecov by Sentry. |
…e-duplicate-resolving-issues
|
Marked as draft until #599 is merged. |
|
Dropping myself from review until other stuff is merged, conflicts are resolved, etc |
…ted/pydoctor into 661-name-duplicate-resolving-issues
…rds imports in the localNameToFullName mapping as well to know wether they are defined or not.
pydoctor/model.py
Outdated
| def isNameDefined(self, name:str) -> bool: | ||
| raise NotImplementedError(self.isNameDefined) |
There was a problem hiding this comment.
Should this become an ABC as well, given that this is really an abstractmethod?
There was a problem hiding this comment.
Yes, it should, but some tests rely on the fact that Documentable can be instantiated standalone... It might be too much refactors to change that.
There was a problem hiding this comment.
Ok, it's more deep that I thought. Since we're not actually using these Documentable classes, but actually dynamically building derived classes in the factory, it actually can't use abc at all, otherwise we're running into the following error:
def add_mixin(self, for_class: str, mixin: Type[Any]) -> None:
super().add_mixin(for_class, mixin)
# Take care to avoid inconsistent MRO by removing extra model.* classes from the Mixin bases.
try:
b = list(mixin.__bases__)
b.remove(getattr(self.model, for_class))
> mixin.__bases__ = tuple(b)
E TypeError: __bases__ assignment: 'ModuleMixin' object layout differs from 'Module'
Please tell me if you have any idea on how to overcome this situation.
In the mean time, I'll merge this PR, thanks.
Edit: I've realized that it's all because I've decided to accept mixin classes that bases the concrete classes for type checking purposes but remove the base class at runtime, and that's why the ABCs are not happy with this situation. The solution would be to stop supporting extending concrete Documentable classes in the mixin. But this would be a major pain for mypy.
pydoctor/model.py
Outdated
| def isNameDefined(self, name:str) -> bool: | ||
| raise NotImplementedError(self.isNameDefined) |
There was a problem hiding this comment.
Yes, it should, but some tests rely on the fact that Documentable can be instantiated standalone... It might be too much refactors to change that.
…ted/pydoctor into 661-name-duplicate-resolving-issues
Fix issues #661 and #662.
Apply rule "Annotations should always be resolved in the context of the module scope"