Skip to content

Rename compute_ngram_distance to _compute_ngram_distance#1838

Merged
rcap107 merged 5 commits into
skrub-data:mainfrom
siddharthbaleja7:mnt-rename-compute-ngram-distance
Jan 14, 2026
Merged

Rename compute_ngram_distance to _compute_ngram_distance#1838
rcap107 merged 5 commits into
skrub-data:mainfrom
siddharthbaleja7:mnt-rename-compute-ngram-distance

Conversation

@siddharthbaleja7

@siddharthbaleja7 siddharthbaleja7 commented Jan 13, 2026

Copy link
Copy Markdown
Contributor

Renames compute_ngram_distance to _compute_ngram_distance to make it private, as requested in #1755.

Updates all internal usages, tests, and examples to use the new name.

@siddharthbaleja7

Copy link
Copy Markdown
Contributor Author

@rcap107 @jeromedockes can you review this pr?

Comment thread examples/0050_deduplication.py Outdated

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should be an entry in the changelog, explaining that the function is now private. Other than that and the other comment I left, the PR looks good.

Comment thread skrub/tests/test_deduplicate.py Outdated


def test_compute_ngram_distance():
def test__compute_ngram_distance():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def test__compute_ngram_distance():
def test_compute_ngram_distance():

@rcap107 rcap107 linked an issue Jan 13, 2026 that may be closed by this pull request
Comment thread CHANGES.rst Outdated
-------

- :func:`compute_ngram_distance` has been renamed to :func:`_compute_ngram_distance` as it is a private function.
:pr:`1755` by :user:`Siddharth Baleja <siddharthbaleja>`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
:pr:`1755` by :user:`Siddharth Baleja <siddharthbaleja>`.
:pr:`1838` by :user:`Siddharth Baleja <siddharthbaleja>`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the changelog should refer to the PR that is addressing the issue, not to the issue

@rcap107

rcap107 commented Jan 13, 2026

Copy link
Copy Markdown
Member

FYI @siddharthbaleja7 if a comment is marked as "suggestion" on GitHub you can commit the suggestion directly by clicking on the button, rather than having do it manually in the code:
image

It's not a big deal, just more convenient.

@siddharthbaleja7

Copy link
Copy Markdown
Contributor Author

Thanks for the tip! I'll definitely use that next time.

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot @siddharthbaleja7

@rcap107 rcap107 merged commit d6e4446 into skrub-data:main Jan 14, 2026
29 checks passed
rcap107 added a commit to rcap107/skrub that referenced this pull request Feb 9, 2026
…1838)

Co-authored-by: Riccardo Cappuzzo <riccardo.cappuzzo@gmail.com>
rcap107 added a commit that referenced this pull request Feb 10, 2026
Co-authored-by: Riccardo Cappuzzo <riccardo.cappuzzo@gmail.com>
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.

Make the function compute_ngram_distance private

2 participants