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

Misc Toolshed tweaks #4990

Merged
merged 7 commits into from
Mar 24, 2024
Merged

Conversation

ElectroJr
Copy link
Member

@ElectroJr ElectroJr commented Mar 23, 2024

  • Added an ICommonSession parser
  • Toolshed commands now only generate completions for the trailing arguments. Previously it generated completions whenever an argument failed to parse, resulting in it generating nonsensical completions.
  • Fixed the CommandDiscriminator equality override not actually overriding the default record struct equality. This was causing it to constantly add new entries to the _concreteImplementations dictionary.
  • Removes an obsolete & broken not-implemented exception actually it appears to still be required
  • Adds ILocalizationManager to parsers & commands.

Copy link
Contributor

@moonheart08 moonheart08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once these are addressed this should be good to go.

Robust.Shared/Toolshed/Syntax/Expression.cs Outdated Show resolved Hide resolved
Robust.Shared/Toolshed/ToolshedCommandImplementor.cs Outdated Show resolved Hide resolved
Robust.Shared/Toolshed/ToolshedCommandImplementor.cs Outdated Show resolved Hide resolved
Robust.Shared/Toolshed/TypeParsers/SessionTypeParser.cs Outdated Show resolved Hide resolved
Robust.Shared/Toolshed/TypeParsers/SessionTypeParser.cs Outdated Show resolved Hide resolved

if (parameters.TryGetValue(subCmd ?? "", out var existing))
{
if (!existing.SequenceEqual(existing))
Copy link
Member Author

@ElectroJr ElectroJr Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously bugged because existing.SequenceEqual(existing) is always true. After I fixed it, many commands started erroring so I though the feature was implemented, but I misunderstood it.

If I understand correctly now, it works as long as the (sub)command name & piped type are unique, so I changed the parameters dictionary key to a (string, Type?) tuple

@metalgearsloth metalgearsloth merged commit b4c1618 into space-wizards:master Mar 24, 2024
4 checks passed
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.

3 participants