Refactoring aliases to be more generic, add function support

[cmbrose]

A few usability improvements for artifacts-helper after digging into edge cases with the feature in codespaces

• Add support to create functions instead of aliases (aliases have some issues with sourceing that functions don't)
• Add support for specifying target rc files, instead of the default bash and zsh files
• Refactor writing the aliases so that it just loops over an array instead of doing each individually
• Add an option to default disable all aliases, so that you don't have to disable them all individually

[markphip]

Once you get the tests working, I would suggest adding to the notes.md file with any explanation of how the functions should be used ... assuming there is some user-facing work to do.

It is not really clear to me if this is meant to replace the aliases or supplement them to support a different scenario. If they can replace the functionality without impact, then why not just drop the option and make this the new way to do things? If they are doing something different than why not just make this an additive option as opposed to replacing?

[markphip]

In general, this is all probably fine. But I have a couple comments and questions:

1. Let's change the version to 2.0.0. Even though I think this is ultimately a safe change, given that it is hard to do real world testing it makes sense to just up the major version number since we are changing the fundamental way this works
2. Switching from alias to function. I think I understand this. For the primary use case it should not result in any change in behavior, and it makes some other custom scenarios a bit easier to do
3. New option for defaulEnableAlias. I cannot figure out what adding this accomplishes. What is the point in this feature if all the aliases are disabled? I would suggest removing this, but if it stays it needs to have a section added to NOTES.md to explain why it exists and when you would use it
4. New targetFiles option. I can guest what maybe this is for, but it needs to be spelled out in NOTES.md as documentation

[cmbrose]

Thanks for the reviews @markphip - I'll try and address all the questions here, but let me know if I missed anything :)

Some background for this change:

For codespaces repos we ran into issues with the auth helper not working for an edge case where we ran npm commands inside a dotnet build command. Commands run by dotnet build are use non-interactive shells and don't read any bashrc type files, so the npm commands weren't using the aliases and getting auth errors.

Even if I explicitly sourced the bashrc files I ran into 2 issues:

• default bashrc files have an explicit check to NOT run in non-interactive shells so even when sourced, they just exit immediately and no-op
• even hacking that away, by default aliases are not exported unless you set shopt expand_aliases which is a fragile to always remember to do

What I ended up doing was to convert all the aliases into functions and add them to a standalone auth helper script (not the bashrc files). That way I could source this standalone file easily and not need to use shopt to make it work.

So to directly answer some questions from experience:

• Why function instead of alias?
  • Functions are easier to use because they are exported by default in bash while aliases require a shopt
• Why not just always use function?
  • This was mostly for back-compat - in case there was an edge case I hadn't considered where aliases worked an functions didn't I wanted users to be able to have a bail out back to aliases
• Why not functions and aliases side by side?
  • This just doesn't work because the function and alias names would collide (since we're shadowing dotnet, npm, etc there's only one name)
• Why the defaultEnableAlias option?
  • By default all aliases are enabled so you have to explicitly opt out of them. When I made the standalone auth helper I had to explicitly opt out of every alias when was 1) a hassle in the config and 2) fragile to new versions of the feature if new aliases were added and not opted-out. So I created this parameter to allow users to switch aliases to an opt-in model instead of opt-out
• What is targetFiles?
  • This would be a custom location to write the aliases to - in my case it would be the standalone auth helper file (so that I didn't need to hand craft it and check it in)

[Copilot]

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 suggestion.

Files not reviewed (2)

• src/artifacts-helper/devcontainer-feature.json: Language not supported
• src/artifacts-helper/install.sh: Language not supported

Comments skipped due to low confidence (3)

src/artifacts-helper/NOTES.md:2

• [nitpick] The phrase 'which dynamically sets an authentication token' should be 'that dynamically sets an authentication token' for consistency.

and optionally configures functions which shadow `dotnet`, `nuget`, `npm`, `yarn`, and `rush` which dynamically sets an authentication token

src/artifacts-helper/NOTES.md:3

• [nitpick] The phrase 'for pulling artifacts from a feed before running the command' is repeated and might be redundant.

for pulling artifacts from a feed before running the command.

src/artifacts-helper/README.md:36

• [nitpick] The term 'shadow' might be unclear to some users. Consider using 'override' or 'replace' instead.

and optionally configures functions which shadow `dotnet`, `nuget`, `npm`, `yarn`, and `rush` which dynamically sets an authentication token

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[Copilot]

The description for targetFiles should mention that it writes functions instead of aliases.

Suggested change

	| targetFiles | Comma-separated list of files to write aliases to | string | DEFAULT (`/etc/bash.bashrc,/etc/zsh/zshrc` for `root`, otherwise `~/.bashrc,~/.zshrc`) |
	| targetFiles | Comma-separated list of files to write functions to | string | DEFAULT (`/etc/bash.bashrc,/etc/zsh/zshrc` for `root`, otherwise `~/.bashrc,~/.zshrc`) |