Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2659: Use get_hg_branch() to get Mercurial branch information. #2660

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

vsajip
Copy link
Contributor

@vsajip vsajip commented Jan 29, 2022

No description provided.

@daxgames
Copy link
Member

daxgames commented Jan 29, 2022

@vsajip can you please take a screenshot and paste it into a comment so we can see the prompt before and after your modification?

I not use mercurial so I have no way to test.

@vsajip
Copy link
Contributor Author

vsajip commented Jan 29, 2022

I can do that, but the prompt hasn't actually changed - only the internal implementation of it has. The original code directly did an io.popen in hg_prompt_filter(). The updated code calls get_hg_branch(), which does the io.popen.

@vsajip
Copy link
Contributor Author

vsajip commented Jan 29, 2022

ss3

In the above screenshot, the last line shows how the prompt looks in a directory with a project using Mercurial as the VCS.

@daxgames
Copy link
Member

daxgames commented Jan 30, 2022

@vsajip I am not trying to be difficult but why change it then?

What is the benefit of your 'fix' over what the current code does if the final result does not change?

Just trying to understand.

@vsajip
Copy link
Contributor Author

vsajip commented Feb 1, 2022

Well, clink.lua contains functions such as get_svn_dir(), get_svn_branch(), get_git_dir(), get_git_branch(), get_hg_dir(), get_hg_branch() for all the different VCS types it supports. These functions were originally written to provide specific information - in the case of get_hg_branch(), it is there to provide Mercurial branch information. I have no idea why that function was bypassed, and whether the change in this PR should be made or not depends on how one values consistency and maintainability in assessing software quality. Clearly "it works" is one (perhaps the minimum) level, but one can also aspire to those other qualities I mentioned. To me, it is better to have the consistency of approach across all VCSes rather than the current situation around bypassing get_hg_branch().

@daxgames
Copy link
Member

daxgames commented Feb 1, 2022

I agree completely was just wondering if you were trying to fix an issue you saw.

Thamks for the PR

@vsajip
Copy link
Contributor Author

vsajip commented Feb 1, 2022

You're welcome. Thanks for your part in maintaining cmder, it makes the Windows command line so much better! I have some local changes to my cmder setup, and every time I update cmder, I need to retrofit those changes into my newly updated cmder. One of those is support for Mercurial and I came across this when doing my retrofit after installing cmder 1.3.19.

There are some other cosmetic things that I do (e.g. spaces between the (branch) and the rest of the prompt, and placement and formatting of the Python virtual environment name in the prompt). I don't know if the cmder dev team would be interested in those. I also have some ideas about making the configuration a bit more data driven, so that certain elements could be picked up from configuration files instead of being hardcoded in Lua scripts. I'd be interested in your thoughts ...

@daxgames
Copy link
Member

daxgames commented Feb 2, 2022

@vsajip v1.3.19 has support for a configurable prompt.

I am curious to know what you mean by retrofitting after upgrade. What kind of changes are you making and how do they change the appearance of the shell?

I am wondering if your retrofits could be made configurable items in newer Cmder versions.

We are always interested in user contribution but we need to be careful not to undo something current users depend on.

@vsajip
Copy link
Contributor Author

vsajip commented Feb 2, 2022

What kind of changes are you making and how do they change the appearance of the shell?

No changes to the appearance of the shell. The two main changes are:

  • The prompt itself (I customise the exact prompt that I want - so single line, no lambda character etc.)
  • I change the get_hg_branch() function to call hg prompt - an open source Mercurial extension I have installed - which provides better branch information than just "hg branch" as used in the vanilla get_hg_branch().

I am wondering if your retrofits could be made configurable items in newer Cmder versions.

Definitely. IMO both the prompt itself and the command to get branch information could be made configurable. On Linux I use a project called bash_git_prompt to provide an enhanced prompt for when you're under a Git project directory. I feel cmder could use some of that project's approach to enhance the Git information provided.

We are always interested in user contribution but we need to be careful not to undo something current users depend on.

Absolutely, I understand the need for backward compatibility. I'm a committer on the Python project, which takes it very seriously, and we sometimes have to reject suggested improvements where they can't be made in a backwards-compatible way.

@vsajip
Copy link
Contributor Author

vsajip commented Feb 2, 2022

v1.3.19 has support for a configurable prompt.

If you mean cmder_prompt_config.lua, I'm aware of it.

@daxgames
Copy link
Member

daxgames commented Feb 2, 2022

  • The prompt itself (I customise the exact prompt that I want - so single line, no lambda character etc.)

The above and more is already possible for Cmder v1.3.19 Cmd.exe prompts. See this or go straight to the config file %cmder_root%/config/cmder_prompt_config.lua.

@vsajip
Copy link
Contributor Author

vsajip commented Feb 2, 2022

Thanks, I'm using that file now. I'll look to see how I can minimise any retrofitting I need to do in this area.

@daxgames daxgames merged commit 0616ff0 into cmderdev:master Feb 3, 2022
@daxgames
Copy link
Member

daxgames commented Feb 3, 2022

Thanks - Merged

@vsajip vsajip deleted the fix-2659 branch February 4, 2022 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants