wisetime-io / vscode-branch-in-window-title

Visual Studio Code extension that shows the current Git branch in the window title
Apache License 2.0
12 stars 6 forks source link

Fixed branch name acquiring #23

Closed AnrDaemon closed 2 months ago

AnrDaemon commented 4 months ago

Read branch name from internal Git API if available, else read the output of git command. This improves, for example, the support for Git workspaces without any extra work.

AnrDaemon commented 4 months ago

Looks like I've reimplemented #22 on a different level.

koenlek commented 3 months ago

Any updates on this? Would love to see this merged if it fixes #12

StK88 commented 3 months ago

Hi,

About vscode git extension:

Git extension types file is added manually, in case of git extension API changes it might become incompatible.

Does https://github.com/wisetime-io/vscode-branch-in-window-title/pull/22 solve your issues? Commented it as well.

AnrDaemon commented 3 months ago

Yes, it is an official way of calling Git extension. In its absence, there's a fallback of direct call to "git" executable. Types file is added for easier development. I could have excluded it from commit, but that would only make it harder for those who would want to work with it in the future. If anything changes in the future, we'd just plop a new definitions file in there and check for any discrepancy.

AnrDaemon commented 3 months ago

And yes, it solved my issue of multiple workspaces for the same repo.

StK88 commented 3 months ago

Thanks for the clarifications. There's no problem with having a git extension types file, the main concern is if the API/types of git extension will change, there's a low probability of a run time error from git extension without reaching the native git fallback. The current approach looks safe since at least extension.exports.getAPI(1) specifies the API version number, but in this case, we need to rely on backward-compatibility of future updates in https://github.com/microsoft/vscode/blob/main/extensions/git/src/api/git.d.ts.

Is there a significant benefit of involving git extension? Theoretically, git extension and native git fix the same issues, but git extension might require manual sync/changes in future.

Minor note, branchDetector tests are failing:

Screenshot 2024-04-10 at 11 32 50

but I wouldn't like you to spend more time on it if we will go with https://github.com/wisetime-io/vscode-branch-in-window-title/pull/22 which uses only native git and solves same issues.

AnrDaemon commented 3 months ago

Accessing API is an order of magnitude faster compared to spawning git executable. (For a given task, I mean.) Also, in case of web-version and remote development, extension could be the only available way of accessing the repo.

AnrDaemon commented 3 months ago

I'm not 100% sure I fixed all tests. If you could give me a hand in this, your help would be greatly appreciated. As it is, I'm not a JS/TS developer.

koenlek commented 3 months ago

@StK88 , FWIW: all my work is on remote systems and this change would finally make this extension usable for me (I've been wanting this for a long time!).

Also, on the extension vs native git call discussion: arguably, going the extension route is perhaps more idiomatic than using native git, assuming the extension is binding git library APIs, whereas native git presumably runs it as a system call (shell command), which can be considered an anti-pattern (but without a doubt different people will have different opinions on that).

StK88 commented 2 months ago

@AnrDaemon @koenlek, thanks for the feedback. Extended implementation with git extension makes sense. I had a look into failing tests and couldn't find a quick way to fix them. I'm unable to dedicate more time to fixing tests. https://github.com/wisetime-io/vscode-branch-in-window-title/pull/22 can be used as a tip/reference with fixed tests.

StK88 commented 2 months ago

@AnrDaemon, can you temporarily give me permission to your fork, so I can push tests fix? Or I can fork your fork and open a separate PR.

StK88 commented 2 months ago

ping @AnrDaemon

AnrDaemon commented 2 months ago

You can PR them to my repo. IDK about giving access.

StK88 commented 2 months ago

@AnrDaemon, opened https://github.com/AnrDaemon/vscode-branch-in-window-title/pull/1 can you please review and merge if all is okay?

AnrDaemon commented 2 months ago

I've had a look over it. We'll need an approval for new dependency first.

vyshane commented 2 months ago

The new dev dependency is fine.

AnrDaemon commented 2 months ago

I think we've resolved the testing issues. Please review.

StK88 commented 2 months ago

@AnrDaemon,

What to VSCode version base, it was changed around the time of https://github.com/wisetime-io/vscode-branch-in-window-title/pull/17 and I frankly don't understand the reasoning. This unwarranted change excluded people using Windows 7 from benefitting by these updates.

Sorry, I don't think supporting Windows 7 is reasonable. Microsoft support for Windows 7 ended in 2020. By keeping the old vscode engine we miss all improvements and bug fixes from newer versions. 1.73.0 is already quite outdated and we will update it in the future anyway. From history, we update it alongside other dependencies. Are you on Windows 7? I'm not sure, maybe Windows version diff was a reason behind test failures since our node versions are quite the same. I'm on Win 10.

AnrDaemon commented 2 months ago

Dropping support without a reason is unreasonable. Extension does not depend on any new API's, bumping dependency version "just because" is a very bad idea in general.

The extension does not package the "old engine", it only contains the code it contains. Which is easily proven by npx vsce ls.

StK88 commented 2 months ago

Yes, it's simply a compatible vscode version.

There are a few reasons:

Even if we leave 1.70.0 downgrade, we can't guarantee that it won't be changed in the future, therefore I asked for a revert, to avoid temporary false expectations and unrelated changes.

AnrDaemon commented 2 months ago

^1.70.0 is not "freeze". It's "1.70.0 <= version < 2.0". So, until VSCode v2 it will be good.

AnrDaemon commented 2 months ago

Thanks for your kind words and your understanding. Is there anything left to attend?

StK88 commented 2 months ago

All is good, thanks for the heads up. We'll merge and release it as soon as possible.