regro / cf-scripts

Flagship repo for cf-regro-autotick-bot
Other
46 stars 70 forks source link

Also try mangling sdist urls to match pep625 #2794

Closed bollwyvl closed 2 weeks ago

bollwyvl commented 2 weeks ago

This adds yet another URL to try, namely the basically-now-ubiquitous PEP625 form:

old    package-name-0.0.0.tar.gz
              V
new    package_name-0.0.1.tar.gz
codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.05%. Comparing base (2c96e01) to head (2bea2c2).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2794 +/- ## ========================================== + Coverage 75.00% 75.05% +0.05% ========================================== Files 109 109 Lines 11156 11181 +25 ========================================== + Hits 8367 8392 +25 Misses 2789 2789 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bollwyvl commented 2 weeks ago

add some tests? I know the tests are thing already. :/

how do we feel about moving a bunch of the black-violating tests to .txt-based fixtures?

beckermr commented 2 weeks ago

What do you mean by that? W/e it is, a new PR please.

beckermr commented 2 weeks ago

I honestly don't get how this PR passes or works. Here are the existing tests for url transforms w/ pypi (which is way more complete than I recall!):

def test_url_transform_pypi():
    urls = set(list(gen_transformed_urls("https://pypi.io/{{ name }}/{{ name }}-barf")))
    assert urls == {
        "https://files.pythonhosted.org/{{ name }}/{{ name }}-barf",
        "https://files.pythonhosted.org/{{ name }}/{{ name.replace('-', '_') }}-barf",
        "https://files.pythonhosted.org/{{ name }}/{{ name.replace('_', '-') }}-barf",
        "https://pypi.io/{{ name }}/{{ name }}-barf",
        "https://pypi.io/{{ name }}/{{ name.replace('-', '_') }}-barf",
        "https://pypi.io/{{ name }}/{{ name.replace('_', '-') }}-barf",
    }

    urls = set(
        list(
            gen_transformed_urls(
                "https://pypi.io/{{ name }}/{{ name.replace('_', '-') }}-barf",
            ),
        ),
    )
    assert urls == {
        "https://files.pythonhosted.org/{{ name }}/{{ name }}-barf",
        "https://files.pythonhosted.org/{{ name }}/{{ name.replace('_', '-') }}-barf",
        "https://pypi.io/{{ name }}/{{ name }}-barf",
        "https://pypi.io/{{ name }}/{{ name.replace('_', '-') }}-barf",
    }

    urls = set(
        list(
            gen_transformed_urls(
                "https://pypi.io/{{ name }}/{{ name.replace('_','-') }}-barf",
            ),
        ),
    )
    assert urls == {
        "https://files.pythonhosted.org/{{ name }}/{{ name }}-barf",
        "https://files.pythonhosted.org/{{ name }}/{{ name.replace('_','-') }}-barf",
        "https://pypi.io/{{ name }}/{{ name }}-barf",
        "https://pypi.io/{{ name }}/{{ name.replace('_','-') }}-barf",
    }

    urls = set(
        list(
            gen_transformed_urls(
                "https://pypi.io/{{ name }}/{{ name|replace('_','-') }}-barf",
            ),
        ),
    )
    assert urls == {
        "https://files.pythonhosted.org/{{ name }}/{{ name }}-barf",
        "https://files.pythonhosted.org/{{ name }}/{{ name|replace('_','-') }}-barf",
        "https://pypi.io/{{ name }}/{{ name }}-barf",
        "https://pypi.io/{{ name }}/{{ name|replace('_','-') }}-barf",
    }

    urls = set(
        list(
            gen_transformed_urls(
                'https://pypi.io/{{ name }}/{{ name.replace("_", "-") }}-barf',
            ),
        ),
    )
    assert urls == {
        "https://files.pythonhosted.org/{{ name }}/{{ name }}-barf",
        'https://files.pythonhosted.org/{{ name }}/{{ name.replace("_", "-") }}-barf',
        "https://pypi.io/{{ name }}/{{ name }}-barf",
        'https://pypi.io/{{ name }}/{{ name.replace("_", "-") }}-barf',
    }

We already test at least part of the - to _ transform, right?

beckermr commented 2 weeks ago

Oh you are munging hard-coded names. duh. Needs tests then for sure.

bollwyvl commented 2 weeks ago

fixtures

url transforms w/ pypi (which is way more complete than I recall!):

Made #2797. Might do a little work there before coming back to here... and indeed, this (draft) PR needs some better stress tests, especially of bad/special/magic cases that categorically did change over time (PEP420 with ., poetry and flit epochs, etc. ).

beckermr commented 2 weeks ago

We have pre-commit so it should be easy to solve linter errors.

beckermr commented 2 weeks ago

pre-commit.ci autofix

bollwyvl commented 2 weeks ago

I am struggling to find any in-the-wild examples that normalize . to _, or apply case normalization.

beckermr commented 2 weeks ago

That's fine. As long as something made up is in the tests, it will be fine I think.

beckermr commented 2 weeks ago

Thank you!