Open msftcangoblowm opened 2 weeks ago
I'm flying blind
Not a Windows user nor have access to a Windows box or VM
This is a plea for help!
On Windows, how to get the absolute path of these executables: sphobjinv and sphobjinv-textconv
These are incorrect
path_soi = Path(sys.executable).parent.joinpath("sphobjinv")
path_soi_textconv = Path(sys.executable).parent.joinpath("sphobjinv-textconv")
Affects
Packages site path: C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\site-packages
site packages: ['c:\hostedtoolcache\windows\python\3.8.10\x64', 'c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages']
user site packages: 'C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\site-packages'
path_scripts: C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\SCRIPTS
I would say, take a look at the other CLI tests and see if it's possible to adapt the approach I took with them to your new tests.
It's possible the needs here are different enough that my existing technique is insufficient, but I'd at least give it a shot first.
(That is, if you haven't already.)
I'm not sure where the script is; looking at the output of this job step, though, it looks like it's in D:\a\1\s
, at least for this run.
You could try the where
command -- it's analogous to which
on Linux.
I'm also not sure why you need to invoke the sphobjinv-textconv
with the full path to the entrypoint script. It should be on path; can't you just call it without any path specified?
Will follow your advice and try with a relative path rather than an absolute path. You are right, the venv bin|SCRIPTS folder should already be on the path
I did some digging and testing -- a separate sphobjinv-textconv
entrypoint that takes the single path argument isn't actually needed.
Combining this Stack Overflow answer with this section of the Git docs, if I add the following to my .gitattributes
file:
*.inv diff=objects_inv
And add this to my .git/config
:
[diff "objects_inv"]
textconv = sh -c 'sphobjinv co plain "$0" -'
It works:
(sphobjinv) C:\git\sphobjinv>git diff
diff --git a/.gitattributes b/.gitattributes
index c6f20db..2a23375 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,2 +1,3 @@
tests/resource/objects_mkdoc_zlib0.inv binary
tests/resource/objects_attrs.txt binary
+*.inv diff=objects_inv
diff --git a/tests/resource/objects_textconv.inv b/tests/resource/objects_textconv.inv
index 31805f5..19a4323 100644
--- a/tests/resource/objects_textconv.inv
+++ b/tests/resource/objects_textconv.inv
@@ -2,6 +2,6 @@
# Project: textconv-test
# Version: 2.3
# The remainder of this file is compressed using zlib.
-sphobjinv.cli.convert py:module 0 cli/implementation/convert.html#module-$ -
-sphobjinv.cli.convert.do_convert py:function 1 cli/implementation/convert.html#$ -
+sphobjinv.cli.convert23 py:module 0 cli/implementation/convert.html#module-$ -^M
+sphobjinv.cli.convert.do_convert234 py:function 1 cli/implementation/convert.html#$ -^M
So, while I really appreciate all of the effort you've gone to here, I believe you can already do what you want with the above recipe.
Open to correction, though, if I'm missing something.
Editing the file manually is easy, via code nontrivial.
On Windows, sphobjinv
executables path must be resolved
On Windows, inventory path must be surrounded by single quotes so the backslashes are not removed by bash resulting in:
C:hostedtoolcachewindowsPython3.8.10x64Scriptssphobjinv-textconv.EXE
On Windows, double quotes does not do the backslash escaping. No idea, if "$0" would work as expected
git config diff.inv.textconv "sh -c 'sphobjinv co plain \"\$0\" -'"
git config diff.inv.textconv
sh -c 'sphobjinv co plain "$0" -'
Bash quote escaping is a nightmare, best avoided. UX wise, this is a nasty ugly non-obvious non-trivial solution requiring advanced bash skills and substantial hair loss (stressful) and code hidden in a config file.
End of the day, the end user needs smooth UX. These details at this level of complexity would have to be hidden.
I used my above-noted setup successfully without modification on both Windows (with Git for Windows) and Debian WSL, so it definitely works across those platforms.
What is the goal of editing the files via code? Providing the setup machinery so a user can run a command and the git textconv is set up automatically for them?
I'm confident (but still possibly wrong!) that the sphobjinv-textconv entry point, and thus also all the new testing around it, are not actually needed.
I like this capability a lot, but I don't want to add an 'automatically set up inv textconv' functionality to sphobjinv. It seems like it would be a bottomless source of support requests and edge case fixes.
It seems to me this is best handled by documenting and publicizing the method for setting it up. Different users will want it set up differently, and so the knowledge will be more valuable than automation here anyways.
A simple python solution is always better than a complicated bash solution. Even if you can, doesn't mean you should
UX wise, either method, but especially the bash method, needs a setup entrypoint. Could add that to soi-textconv. Didn't, to keep the PR scope limited. sphobjinv-textconv --setup
. The bash route, recommend not adding to soi entrypoint. Already complicated.
So it's an UX issue. One command ... done. Much nicer than having to explain the intricacies of a complex sewer system.
What is the goal of editing the files via code?
Simplicity. Zero learning curve. Doing less work. Type one uncomplicated command ... done.
Seems like it would be a bottomless source of support requests and edge case fixes
Internally, soi-textconv runs git config
. No one would have to manually edit the .git/config
(nor the .gitattributes
files) or care about the details.
Besides small changes to the parser, all the code for the setup has already been written and is known to work. Just refactor the integration unittest. After a refactor, the complexity of the integration test should be greatly reduced
The scope of soi-textconv is it configures git to understand inventory files. That scope shouldn't change besides the --setup
and maybe --global
flags.
@msftcangoblowm
TL;DR - You've done a lot of work here, but I'm afraid it's too much... more than I'm willing to add to the project. There's still an avenue to a smaller contribution, though.
I'm on board with:
sphobjinv-textconv
entrypoint to allow the git config entry to be cleaner.inv
textconv
cli/core_textconv.py
makes sense for now, but I might change my mindBut I don't want to uptake anything more than that:
INFILE
argument.sphobjinv
entrypoint should remain the primary, and only configurable, CLI interface surface..gitattributes
or .git/config
.sphobjinv
documentation.So—if you're interested in paring down the PR to just the above (or, starting a new PR to only implement the above, which might be easier/faster), I think there's still a path to a merge here. Let me know if you want to move forward, and I'll leave more thorough comments/feedback on the PR.
I apologize that you went to all this work before getting this feedback, I didn't have any sense before you started that you were envisioning something of this scale. Lesson learned, I should've started a conversation about scale and approach before you started working!
All of this said, I'm still not convinced that the new entrypoint is actually required. Using user-scope .gitconfig
and attributes
files, and installing sphobjinv
with pipx
, I've been able to get the textconv working on both Git for Windows on Win11 and on Debian WSL without having a sphobjinv
-containing virtualenv activated.
In $HOME/.gitconfig
(WSL) & %USERPROFILE%/.gitconfig
(Win11):
[diff "objects_inv"]
textconv = sh -c 'sphobjinv co plain "$0" -'
In $HOME/.config/git/attributes
(WSL) & %USERPROFILE%/.config/git/attributes
(Win11):
*.inv diff=objects_inv
For WSL (similar but nonidentical on Win11):
$ sudo apt install -y pipx
$ pipx install sphobjinv
As we've already discussed, the benefit of sphobjinv-textconv
would be a simpler invocation in the .gitconfig
... but not that much simpler, given that either way, it's two canned snips of config for users to add into the right files.
It's really not much more than a quick post on a blog or Medium or wherever.
(I do understand that a magic command to insert those snips for users would make it even simpler for those users; but, that magic command would end up needing to accommodate a wide variety of operating systems and user setups, and I'm confident it would run into all sorts of hairy edge cases that would be a nightmare to debug and develop around. Far better IMO just to educate users on how to set it up, rather than trying to write something that will attempt to automatically set it up for a wide range of users.)
Regardless, again, if you're still interested in putting together the smaller-scope contribution for adding sphobjinv-textconv
, I'm on board with working with you toward that.
The main sphobjinv entrypoint should remain the primary, and only configurable, CLI interface surface.
If you feel strongly sphobjinv --setup
is better UX than sphobjinv-textconv --setup
, i'm totally on board with whatever results in better UX.
This is actually a good idea that did not occur to me.
If the flag should have another name, just suggest a better flag name.
It should take no options, and only the single INFILE argument.
The --setup
flag has to go somewhere. If it goes onto the sphobjinv CLI, then sphobjinv-textconv
need not be changed any further.
A bit of basic documentation of how to set up the .inv textconv No tutorials on anything git other than brief instructions for setting up the textconv. It's just beyond the scope for the sphobjinv documentation.
On board with simple docs just saying for git diff
support run optional command sphobjinv --setup
. Then not explaining the manual method or the nitty gitty details.
No direct testing of the operation of the git textconv. Nothing new in the test suite driving git or editing .gitattributes or .git/config.
Can move that out of fixtures and into code modules.
So—if you're interested in paring down the PR to just the above
Yes would be interested. Would like to change the current PR rather than making a new PR.
Please confirm i can go ahead.
but, that magic command would end up needing to accommodate a wide variety of operating systems and user setups, and I'm confident it would run into all sorts of hairy edge cases that would be a nightmare to debug and develop around.
When modifying .git/config
, sphobjinv-textconv
calls git config
. So any hairy edge cases could only be caused by upstream. git config
will have a flag controlling whether those changes affect local or globally.
We, the maintainers, need to have read and understood the git config
manual. Specifically how to get/set.
When modifying .gitattributes
, sphobjinv-textconv
is doing that, but it's one line. The only edge case, is whether needs \r\n
or not.
The setup changes are local; affecting one repo. Down the road, someone may request a local/global switch. Global setup goes into ~/.config/git/config
and where ever the global .gitattributes
is located.
There is a Python package specifically for platform xdg user and site folders. The platform specific xdg folders location will not an issue. I already have a wrapper module and unittest for this. So copy paste ... done
Is the PR a fix or a feature? This is a new feature release
Bump the version up to 2.4.0
Describe the changes in the PR Closes #295
Does this PR close any issues? Closes #295
Does the PR change/update the following, if relevant?