Skip to content

Conversation

agnieszka-m
Copy link
Contributor

@agnieszka-m agnieszka-m commented Jul 24, 2024

Related Issues

Proposed Changes:

Standardize and improve docstrings.

How did you test it?

Notes for the reviewer

Checklist

@agnieszka-m agnieszka-m requested a review from dfokina July 24, 2024 07:07
@agnieszka-m agnieszka-m requested a review from a team as a code owner July 24, 2024 07:07
@agnieszka-m agnieszka-m requested review from julian-risch and removed request for a team July 24, 2024 07:07
@github-actions github-actions bot added the type:documentation Improvements on the docs label Jul 24, 2024
@agnieszka-m agnieszka-m added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Jul 24, 2024
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

For the class docstring the suggested changes seem to divert from our current standard. Unless we change our standard, I would keep that line as is. The other changes look good to me. 👍

@component
class SentenceTransformersTextEmbedder:
"""
A component for embedding strings using Sentence Transformers models.
Copy link
Member

@julian-risch julian-risch Jul 24, 2024

Choose a reason for hiding this comment

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

All other components in Haystack use one sentence for this docstring and many (not all) start with A component. If we change only the class SentenceTransformersTextEmbedder it's not really standardized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're going to gradually change them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goal is also to shorten the docstrings and not repeat what's obvious, that's why I did away with "A component for.."

Copy link
Contributor

@dfokina dfokina Jul 24, 2024

Choose a reason for hiding this comment

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

@julian-risch "A component" is used often, but is not a standard (see DocumentWriter, CacheChecker, converters for examples), I would say that the shorter style that describes what component is doing looks quite good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julian-risch are you OK with the new format? can I resolve this?

@julian-risch
Copy link
Member

@dfokina @agnieszka-m I saw that the related PR #8057 mentions the PR is part of a series of changes. I suggest that you open one issue for the series of changes with a description of the required changes.

@agnieszka-m agnieszka-m requested a review from dfokina July 24, 2024 10:30
@coveralls
Copy link
Collaborator

coveralls commented Jul 24, 2024

Pull Request Test Coverage Report for Build 10093246889

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 90.145%

Files with Coverage Reduction New Missed Lines %
components/embedders/sentence_transformers_text_embedder.py 2 94.87%
Totals Coverage Status
Change from base Build 10093234193: 0.0%
Covered Lines: 6860
Relevant Lines: 7610

💛 - Coveralls

@agnieszka-m agnieszka-m requested a review from dfokina July 25, 2024 10:27
@dfokina dfokina requested a review from julian-risch July 25, 2024 11:25
@julian-risch julian-risch changed the title Docs: Standardize and improve docstrings Docs: Standardize and improve SentenceTransformersTextEmbedder docstrings Jul 25, 2024
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 Thank you also for creating the overview issue here #8062 What remains to be done is to better explain to the rest of the team what standards to follow when writing docstrings in future.

@dfokina
Copy link
Contributor

dfokina commented Jul 25, 2024

@julian-risch Absolutely, I'll document the standards for the docstrings.

@dfokina dfokina merged commit 1f58ec2 into main Jul 25, 2024
@dfokina dfokina deleted the sentencetransformerstextembedder-docstrings branch July 25, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes. type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: clean up docstrings of SentenceTransformersTextEmbedder
4 participants