Textconv previously 307

[msftcangoblowm]

rebased local with bskinn/sphobjinv
sync'ed msftcangoblowm/textconv with bskinn/sphobjinv
git push
forced to create new PR

[bskinn]

Ah, I had to approve the workflows since it was a new PR. They should run automatically from here on out.

[bskinn]

And, ok, I've had it with Codecov. I thought I had it configured correctly per their docs, but apparently not.

Please remove this last step from all_core_tests.yml, and codecov from requirements-ci.txt.

[bskinn]

As far as next work steps on the PR, that was in this comment from the prior PR:

---

Code review wise -- I think it'll be best to work on the code first, and then follow with the docs once the code is settled.

---

First, high-level, I'm not sure what the value is for having so much re-implemented in the new textconv code. I don't really want to have two spellings for a bunch of stuff that's already in the original sphobjinv command. I was picturing sphobjinv-textconv to be a very narrow, single-purpose utility:

• Only reading an .inv from disk (so, no --url support)
• No --expand or --contract options
• No 'adapt to survive' logic, really - you provide it a path to a file, it converts it to plaintext on stdout with no other modifications, and that's it.

It also seems like there's a lot of unnecessary/irrelevant logic in .core_textconv.main().

In sum -- I think all of the new functional code should be much slimmer. Is that workable, to make a trim-down refactoring pass on everything?

---

Second, related -- I'm having a hard time understanding the purpose/function of print_stderr_2 and _wrap_inv_stdin. Are they leftovers from the attempt to include live git textconv testing in the test suite? If they're not specifically needed for any remaining tests to function correctly, they should definitely be removed.

(If they are needed for some remaining tests to function correctly, please make an attempt to revise the tests so that they can successfully use the original print_stderr().)

---

Thanks!

[msftcangoblowm]

Done with two lines of code in 61519e6

    is_quiet = params.get(PrsConst.QUIET, False)
    subparser_name = params.get(PrsConst.SUBPARSER_NAME, None)

    if not is_quiet or (subparser_name is None or subparser_name[:2] == "su"):
        print(thing, file=sys.stderr, end=end)

Changing function signature was not needed

CAN IGNORE EVERYTHING BELOW HERE

print_stderr is being passed all soi params, but only requires two. Should be refactored as optional kwargs with sane defaults.

Once this fix is deployed, print_stderr_2 and usage of unittest.mock.patch are no longer needed. And both soi and soi-textconv are more maintainable and faster.

How to defang print_stderr

Instead of passing in params dict (hated by static type checkers), accept only optional keyword args.

def print_stderr(thing, params, *, end="\n"):

becomes

in cli/ui.py

from __future__ import annotations

import os
import sys

def print_stderr(
    thing,
    *,
    subparser_name: str | None = None,
    is_quiet: bool | None = False,
    end: str | None = os.linesep,
):
    """doc str here"""
    # Harden against unexpected user input
    if subparser_name is not None and not isinstance(end, str):
        subparser_name = None

    #    bool not optional bool
    if is_quiet is None or not isinstance(end, bool):
        bool_is_quiet = False
    else:
        bool_is_quiet = is_quiet

    if end is not None and not isinstance(end, str):
        end = os.linesep

    if (
        not bool_is_quiet
        or (subparser_name is None or subparser_name[:2] == "su")
    ):
        print(thing, file=sys.stderr, end=end)

[msftcangoblowm]

Here is a working example of how to post to codecov. Use the gh action, do not reinvent the wheel.

    - name: "Run coverage"
      run : |
        set -xe
        python -m coverage run --parallel -m pytest --showlocals --quiet tests
        python -m coverage combine
        python -m coverage xml --fail-under=95

    - name: "Upload to Codecov"
      uses: codecov/[email protected]
      with:
        name: unittest-py${{ matrix.python-version }}
        token: ${{ secrets.CODECOV_TOKEN }}
        slug: msftcangoblowm/wreck
        os: linux
        flags: unittests
        files: ./coverage.xml
        env_vars: OS,PYTHON
        fail_ci_if_error: true
        verbose: true

Whats known to work ftw!

• the slug should be bskinn/sphobjinv

• then setup the codecov badge if don't already have one

• matrix.python-version is not set, so unavailable. Add py version to matrix or hard code it?

Would it be ok to use codecov/codecov-action? Ignoring coverage vs pytest-cov

commit

[msftcangoblowm]

_wrap_inv_stdin

soi-textconv is only for use with git.

If git supplied it by redirect then _wrap_inv_stdin is the workaround.

soi-textconv is written to be UNIX standard utility. UNIX utils are expected to deal with pipes, redirects, or as cli options. I assume, git to expect textconv to be UNIX standards compliant utility. Then it's up to git on how to interact with the textconv. That's a git internal implementation decision.

[msftcangoblowm]

added fix for all_core_tests.yml. codecov package removed from requirement-ci.txt

codecov should work

[bskinn]

added fix for all_core_tests.yml. codecov package removed from requirements-ci.txt.

Looks like there's a syntax error in the new YAML.

That said, please just remove the Codecov job step, instead of taking the time to fix it. Codecov is really for larger projects and large teams, where you want team-wide visibility and/or accountability over coverage. It's overkill for sphobjinv ... I added it a while back because I was curious about it, and curious if I could get it to work, and I never really got much value out of it.

[bskinn]

_wrap_inv_stdin

soi-textconv is only for use with git.

If git supplied it by redirect then _wrap_inv_stdin is the workaround.

soi-textconv is written to be UNIX standard utility. UNIX utils are expected to deal with pipes, redirects, or as cli options. I assume, git to expect textconv to be UNIX standards compliant utility. Then it's up to git on how to interact with the textconv. That's a git internal implementation decision.

Per the Git docs:

The program should take a single argument, the name of a file to convert, and produce the resulting text on stdout.

The input API contract for the textconv is specifically a string filename passed as a CLI argument. I suppose that may change someday, but I'd rather postpone uptaking that complexity to the project unless/until it's specifically needed. Please pare back to only this use case.