Skip to content

Conversation

@sdn4z
Copy link
Collaborator

@sdn4z sdn4z commented Oct 2, 2025

closes #329

@sdn4z sdn4z marked this pull request as ready for review October 2, 2025 08:40
@sdn4z sdn4z requested a review from scastlara as a code owner October 2, 2025 08:40
@scastlara
Copy link
Collaborator

/lgtm review

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🦉 lgtm Review

Score: Nitpicks 🤓

🔍 Summary

This PR adds much-needed docstrings across the codebase, which is a great improvement. For the most part, the documentation is clear and helpful. In the future, please ensure the PR title and description accurately reflect all changes, as this PR also includes some refactoring. I have left a couple of minor comments on docstrings that are either incorrect or could be more descriptive. After addressing these, the PR will be in good shape to merge.

More information
  • Id: 4dd237e8f9a647959642f28aefd83d38
  • Model: gemini-2.5-pro
  • Created at: 2025-10-02T08:47:21.148780+00:00
Usage summary
  • Request count: 2
  • Request tokens: 45,071
  • Response tokens: 12,788
  • Total tokens: 57,859

See the 📚 lgtm-ai repository for more information about lgtm.

from twyn.trusted_packages.selectors import AbstractSelector

_PackageNames = defaultdict[str, set[str]]
"""Type alias for mapping package names by ecosystem."""
Copy link
Contributor

Choose a reason for hiding this comment

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

🦉 🎯 Correctness

Severity: MEDIUM 🟡

"""Type alias for mapping package names by ecosystem."""

This docstring appears to be incorrect. Based on its usage, the _PackageNames type alias maps the first letter of a package name to a set of package names that start with that letter, not by ecosystem. Please update the docstring to reflect its actual purpose.

Suggested change
"""Type alias for mapping package names by ecosystem."""
"""Type alias for mapping a package's first letter to a set of package names."""
More information about this comment
  • File: src/twyn/trusted_packages/trusted_packages.py
  • Line: 12
  • Relative line: 4
  • With suggestion: Yes
  • Suggestion ready for replacement: Yes

super().__init__(message)

def show(self, file: Optional[IO[Any]] = None) -> None:
"""Display the error message."""
Copy link
Contributor

Choose a reason for hiding this comment

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

🦉 ✨ Quality

Severity: LOW 🔵

"""Display the error message."""

This docstring is a bit vague. The method logs the error message, so a more descriptive docstring would be more accurate and helpful.

Suggested change
"""Display the error message."""
"""Logs the error message using the configured logger."""
More information about this comment
  • File: src/twyn/base/exceptions.py
  • Line: 32
  • Relative line: 4
  • With suggestion: Yes
  • Suggestion ready for replacement: Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AI correcting AI. gotta love it

@sdn4z sdn4z merged commit 6a4c703 into elementsinteractive:main Oct 2, 2025
12 checks passed
@sdn4z sdn4z deleted the all-docs branch October 2, 2025 12:40
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.

Include docstrings in all methods

2 participants