sphobjinv-textconv #301
sphobjinv-textconv #301
Conversation
bskinn
added
type: enhancement ✨
|
Sorry for the delay here, needed to release #299 to make the package play nicer with Debian's build process, and then the holidays hit. First thing -- your fork's
It's possible you didn't push your updated |
conftest.py
Outdated
| @@ -82,6 +82,51 @@ def res_dec(res_path, misc_info): | |||
| return res_path / (misc_info.FNames.RES.value + misc_info.Extensions.DEC.value) | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="session") | |||
| def res_cmp_plus_one_line(res_path, misc_info): | |||
There was a problem hiding this comment.
Is this fixture needed any more, since this PR will only be adding the sphobjinv-textconv endpoint and not actually doing anything with git diffs?
conftest.py
Outdated
| @@ -264,6 +303,68 @@ def func(arglist, *, expect=0): # , suffix=None): | |||
| return func | |||
|
|
|||
|
|
|||
| @pytest.fixture() # Must be function scope since uses monkeypatch | |||
| def run_cmdline_textconv(monkeypatch): | |||
There was a problem hiding this comment.
Instead of duplicating this fixture with a different hard-coded command, I would rather generalize the existing run_cmdline_test() by having it not assume what the command is, and have all of the points-of-use of the fixture just pass in the command along with the other arguments.
(It's possible that this isn't feasible, I haven't looked closely. Interested in your thoughts.)
conftest.py
Outdated
|
|
||
| @pytest.fixture() # Must be function scope since uses monkeypatch | ||
| def run_cmdline_no_checks(monkeypatch): | ||
| """Return function to perform command line. So as to debug issues no tests.""" |
conftest.py
Outdated
| @@ -152,19 +197,13 @@ def scratch_path(tmp_path, res_path, misc_info, is_win, unix2dos): | |||
| # With the conversion of resources/objects_attrs.txt to Unix EOLs in order to | |||
| # provide for a Unix-testable sdist, on Windows systems this resource needs | |||
| # to be converted to DOS EOLs for consistency. | |||
| if is_win: | |||
| if is_win: # pragma: no cover | |||
There was a problem hiding this comment.
I'm confused as to why these lines started showing up in any of the coverage testing. My pipelines don't show either setup.py or conftest.py, in either the GH Action for testing the main code or the Azure Pipeline for checking execution of all the test code.
These pragmas shouldn't be necessary.
There was a problem hiding this comment.
Is this file needed if we're not directly testing git diff with textconv?
There was a problem hiding this comment.
Is this file needed if we're not directly testing git diff with textconv?
|
Dear Brian,
Was wondering why the commits/PR was taken out of chronological order. As long as there is a good reason.
First thing -- your fork's main still isn't current with upstream
Before a merge, since #299 occurred, so will have to rebase [msftcangoblowm:textconv](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)Might be able to just rebase [msftcangoblowm:textconv](https://github.com/msftcangoblowm/sphobjinv/tree/textconv) and continue ignoring [msftcangoblowm:](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)main
[msftcangoblowm/sphobjinv:textconv](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)Have never done a serious rebase
It's possible you didn't push your updated main to your fork
That's most definitely the case and might not matter at all. What's important is your main. And keeping up to date with your main. That's what a rebase will solve.
Taken directly from the #301 PR merge page
`[msftcangoblowm](https://github.com/msftcangoblowm) wants to merge 4 commits into [bskinn:main](https://github.com/bskinn/sphobjinv/tree/main) from [msftcangoblowm:textconv](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)`
Working in a branch is normal.
After the merge, can resync both [msftcangoblowm:](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)main and [msftcangoblowm:textconv](https://github.com/msftcangoblowm/sphobjinv/tree/textconv)
Will do the rebase and message you (on mastodon) once that's done.
Dave
…On Saturday, December 28th, 2024 at 5:41 AM, Brian Skinn ***@***.***> wrote:
Sorry for the delay here, needed to release [#299](#299) to make the package play nicer with Debian's build process, and then the holidays hit.
First thing -- your fork's main still isn't current with upstream, which might be the cause of the merge conflict:
[image.png (view on web)](https://github.com/user-attachments/assets/a316eb30-cba8-4d2a-b6e9-3f041a60da84)
It's possible you didn't push your updated main to your fork? If git diff origin/main..upstream/main shows any changes, then something's off.
—
Reply to this email directly, [view it on GitHub](#301 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AVSZJ2OTBANFJ65MKXUFA732HY2YBAVCNFSM6AAAAABTPA6ZLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRUGIYDANRSGE).
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
msftcangoblowm
force-pushed
the
textconv
branch
from
ff23404 to
b30591c
Compare
Is the PR a fix or a feature?
feature
Describe the changes in the PR
Adds entrypoint sphobjinv-textconv
Does this PR close any issues?
Closes #295
Does the PR change/update the following, if relevant?
azure-pipelines adjust coverage fail under reduced 100% --> 90%. Purposefully in a separate commit, for later easy adjustment via
git rebaseNo CHANGELOG entry. No adjustment to version
pyproject.tomlreformatted by pre-commitUnit tests. No integration test. Some fixtures added to
conftest.pyMinimal adjustments to
doc/conf.pyto pass linkcheck.# pragma: no coverused within the entrypoint, not for non soi-textconv modules. In non soi-textconv modules, using# pragma: no coverwould return coverage to ~99%.[*] Documentation
[*] Tests
[ ] CHANGELOG