Alias collision

[hofbi]

Standards checklist:

• The PR title is descriptive.
• The PR doesn't replicate another PR which is already open.
• I have read the contribution guide and followed all the instructions.
• The code follows the code style guide detailed in the wiki.
• The code is mine or it's from somewhere with an MIT-compatible license.
• The code is efficient, to the best of my ability, and does not waste computer resources.
• The code is stable and I have tested it myself, to the best of my abilities.

Changes:

Relates #10857

This is a first draft checking for alias collisions. I used Python to implement this feature to provide a solid set of tests and added a job to the GH actions

Other comments:

Since this is a first draft there a some open points to discuss:

1. I added everything related to this check into the tools folder. Is this locations fine or should we move it somewhere else?
2. As a first draft, I kept the regex simple. I would first try to get the basic feature in and then improve over time.
3. Already with this basic feature, the script finds some alias collisions. That's why the CI fails right now. How should we proceed here? Disable the CI job for now and fix the collisions first or fix them as part of this PR?

[mcornella]

I think we need to make this a zsh check to account for dynamic alias definitions and function conflicts (i.e. "aliases" that are defined as functions, like #10851). A way to do this could be to run zsh, then parse the output of alias and print -l ${(k)functions}, though this could potentially be super slow if we need to source all plugins.

The problem of side-effects and running extra code (things not related to aliases and functions) would need to be considered as well. I'll think carefully about this last part.

Good start though, love that it contains tests!

[hofbi]

Is there an easy way to source all plugins? Then I would give it a try to see how the runtime will behave.

The current implementation already found a few duplicates. How should we proceed here? I could create a separate issue for every plugin to kickof a discussion for resolving the name conflict.

[hofbi]

@mcornella How should we proceed here? I could create a separate issue for every plugin to kickoff a discussion for resolving the name conflict. We anyhow have to resolve the name conflicts before we can use the CI check in a reasonable way.

And how to proceed with the implementation. Since this already found some conflicts and might prevent future ones, we could already integrate it and improve with a follow up PR that parses the output of alias and print -l ${(k)functions}. What do you think?

[hofbi]

@mcornella How should we proceed here? Is this still relevant for you?

[carlosala]

I'd push for a zsh check, so we can check also functions. Another problem might be that some aliases are defined only if some binary is present in $PATH. Maybe our best option is to go for regex checking all the files looking for alias <word>, function <word> (or <words>), <word> () { and then if there are two with the same name.
What do you think? @hofbi @mcornella

[hofbi]

@carlosala Agreed.

My suggestion on how to proceed would be to go with this check for now and fix everything that was catched so far. Once this is done, we can extend the check for functions and everything else that comes to our mind.

I am also starting list here with alias collisions we need to solve and my suggestions/ideas to resolve them:

1. Alias ma=meteor add defined in meteor.plugin.zsh already exists as alias ma=mongocli atlas in mongocli.plugin.zsh. --> My suggestion, rename all mongocli from m to mc which results in mca
2. Alias db=deno bundle defined in deno.plugin.zsh already exists as alias db=dotnet build in dotnet.plugin.zsh. --> My suggestion, rename all dotnet from d to dn which results in dnb
3. Alias gcd=git checkout $(git config gitflow.branch.develop) defined in git-flow.plugin.zsh already exists as alias gcd=git checkout $(git_develop_branch) in git.plugin.zsh. --> Can we use the git plugin function in the git-flow function?
4. Alias map=web_search duckduckgo \!m defined in web-search.plugin.zsh already exists as alias map=meteor add-platform in meteor.plugin.zsh. --> Rename map to maps?
5. Alias sd=ruby script/server --debugger defined in rails.plugin.zsh already exists as alias sd=ruby script/destroy in rails.plugin.zsh. --> This is under legacy stuff so maybe consider removing it?
6. Alias sp=ruby script/plugin defined in rails.plugin.zsh already exists as alias sp=web_search startpage in web-search.plugin.zsh. --> Also one in rails legacy so consider removing it
7. Alias rs=repo sync defined in repo.plugin.zsh already exists as alias rs=rails server in rails.plugin.zsh.
8. Alias ru=repo upload defined in repo.plugin.zsh already exists as alias ru=rails runner in rails.plugin.zsh.
9. Alias n="$GREP" -Rvi defined in singlechar.plugin.zsh already exists as alias n=nanoc in nanoc.plugin.zsh.
10. Alias a=echo >> defined in singlechar.plugin.zsh already exists as alias a=ansible in ansible.plugin.zsh.
11. Alias c=cat defined in singlechar.plugin.zsh already exists as alias c=composer in composer.plugin.zsh.
12. Alias p="$PAGER" defined in singlechar.plugin.zsh already exists as alias p=ps -f in common-aliases.plugin.zsh.
13. Alias m=man defined in singlechar.plugin.zsh already exists as alias m=meteor run in meteor.plugin.zsh. --> Rename m to mr?
14. Alias sf="$ROOT" "$GREP" -Rli defined in singlechar.plugin.zsh already exists as alias sf=_symfony_console`` in symfony2.plugin.zsh.
15. Alias sc="$ROOT" cat defined in singlechar.plugin.zsh already exists as alias sc=ruby script/console in rails.plugin.zsh.
16. Alias sp="$ROOT" "$PAGER" defined in singlechar.plugin.zsh already exists as alias sp=web_search startpage in web-search.plugin.zsh.
17. Alias sd="$ROOT" "$WGET" defined in singlechar.plugin.zsh already exists as alias sd=ruby script/destroy in rails.plugin.zsh.
18. Alias afs=apt-file search --regexp defined in ubuntu.plugin.zsh already exists as alias afs=apt-file search --regexp in debian.plugin.zsh.
19. Alias allpkgs=dpkg --get-selections | grep -v deinstall defined in ubuntu.plugin.zsh already exists as alias allpkgs=aptitude search -F "%p" --disable-columns ~i in debian.plugin.zsh.
20. Alias mydeb=time dpkg-buildpackage -rfakeroot -us -uc defined in ubuntu.plugin.zsh already exists as alias mydeb=time dpkg-buildpackage -rfakeroot -us -uc in debian.plugin.zsh.
21. Alias mis=microk8s.istio defined in microk8s.plugin.zsh already exists as alias mis=meteor install-sdk in meteor.plugin.zsh.
22. Alias jh=juju help defined in juju.plugin.zsh already exists as alias jh=jhbuild in jhbuild.plugin.zsh.
23. Alias zcl=sudo zypper cl defined in suse.plugin.zsh already exists as alias zcl=sudo zypper clean in suse.plugin.zsh.
24. Alias h=history defined in history.plugin.zsh already exists as alias h=history in common-aliases.plugin.zsh.
25. Alias rn=react-native defined in react-native.plugin.zsh already exists as alias rn=rails notes in rails.plugin.zsh.
26. Alias dr=docker container run defined in docker.plugin.zsh already exists as alias dr=dotnet run in dotnet.plugin.zsh.
27. Alias rake=noglob rake defined in rake.plugin.zsh already exists as alias rake=_rake_command in rails.plugin.zsh.
28. Alias github=frontend github defined in frontend-search.plugin.zsh already exists as alias github=web_search github in web-search.plugin.zsh.
29. Alias stackoverflow=frontend stackoverflow defined in frontend-search.plugin.zsh already exists as alias stackoverflow=web_search stackoverflow in web-search.plugin.zsh.
30. Alias hs=hanami server defined in hanami.plugin.zsh already exists as alias hs=history | grep in history.plugin.zsh.
31. Alias rubies=rvm list rubies defined in rvm.plugin.zsh already exists as alias rubies=chruby in chruby.plugin.zsh.

[TriMoon]

@hofbi
3. Already with this basic feature, the script finds some alias collisions. That's why the CI fails right now. How > should we proceed here? Disable the CI job for now and fix the collisions first or fix them as part of this PR?

I would vote for "fix them as part of this PR" 😉
Good job so far with this cleanup guys, im sure it's a lot of work... 👍

[hofbi]

@carlosala What is you opinion on how to proceed with the existing alias collisions?

[mcornella]

Hey Markus, the technical work so far is spotless but I'm not sure how we proceed with the aspect of solving collisions. Perhaps we can proceed with a CI approach that is "this PR does not introduce new collisions", kinda like how an iterative approach is done with unit testing.

For this, the output of the tool would need to be refined and the job changed so it makes the comparison between master and the PR. I will work on refining the CI part and then we can begin a new issue discussing how to approach current alias collisions.

Thank you for your work!

[carlosala]

I think we can move this forward! I'd go for the "this PR does not add new collisions" thing, and it'd be great to add an automated check as well for collisions with other cli binaries, leveraging something like https://command-not-found.com
@hofbi what do you think about?

[carlosala]

Since this would be a CI-only check, I'm OK going for python. Previously I mentioned something like turning this PR into a shell script, but python is the way to go IMO. 👍🏻

[hofbi]

Sounds good to me. I would leave the binary tool collision for a follow up PR then.

At some point, we could also consider making the part of a tool such as https://pre-commit.com. But I guess you don't want any user/developer to depend on Python.

[carlosala]

Yep, it's probably better to have that only on CI 👍🏻

[hofbi]

@carlosala I slightly refactored this PR to use pytest over unittest, which is just nicer to use. With #10859 (comment), I added a filter that lists all know collisions. Execpt for those, the check will fail for new collisions. In the future we can work on reducing collisions and extend with further checks.

[hofbi]

I removed the known collision resolved in #12192

[hofbi]

@carlosala quick reminder to move on with this

[hofbi]

If I understand the motivation for you approach correctly, you want to avoid storing the json file in VCS, right? On the other hand, having the reference from master in VCS, it is easier to track the status about how many collision you have right now.

Regardless of which way we go for, do you think this could become a follow up PR? This PR, as is provides already value and would prevent new collisions from being introduced.

[carlosala]

Yep, exactly. I'd say it's better not to have it in the repo.
I would like to already merge a final thing, and avoid follow-up PRs.

[hofbi]

@carlosala I updated the GH action. Now we create the baseline file on the source branch. I a second step, we compare the target branch collisions with this baseline.

[carlosala]

Could you move all this code to .github/workflows? Something like https://github.com/ohmyzsh/ohmyzsh/tree/master/.github/workflows/dependencies

[carlosala]

I'll verify the python code during this week, thanks for the hard work!

[hofbi]

Could you move all this code to .github/workflows? Something like https://github.com/ohmyzsh/ohmyzsh/tree/master/.github/workflows/dependencies

Should I moved it into the same folder and merge the requirements.txt or into a seperate one?

[carlosala]

I'd do different ones actually

[hofbi]

Moved it to .github/workflows/alias_collisions

[carlosala]

Some things I spotted along the way:

• We're not really interested in the value itself of the alias, I'd only save the file where is declared and nothing else. My arch for it would be (automatically supporting >2 repeated aliases, which now is an issue:

[
  {
    "alias": "<duplicated>"
	"locations": ["<file1>", "<file2>", "...", "<fileN>"]
  }
]

Main issue with it: if we have to equal aliases in the same file, weird corner cases might appear.

• For the moment we're only matching alias foo='bar', and aliases might be declared without quotes, etc.
• We only match aliases defined in the start of the line, let's remove the ^.

[carlosala]

I'll keep reviewing it during the next days, but really great job!