Use KeyedCollection instead of Dictionary
#130
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xt0rted
force-pushed
the
script-loading
branch
5 times, most recently
from
f5ce0bb to
b8d1128
Compare
aarondandy
reviewed
Dec 5, 2022
| using RunScript.Serialization; | ||
|
|
||
| [JsonConverter(typeof(ScriptCollectionConverter))] | ||
| public class ScriptCollection : KeyedCollection<string, ScriptMapping> |
There was a problem hiding this comment.
I wonder if it might be worth setting the key comparer with the base constructor if you need OrdinalIgnoreCase or Invariant?
| { | ||
| foreach (var script in jsonScripts) | ||
| { | ||
| scripts.Add(script.Name, script.Value.ToString()); |
There was a problem hiding this comment.
Maybe it's possible for a null value to end up in script.Value when deserializing.
src/RunScriptCommand.cs
Outdated
| string[] scripts) | ||
| { | ||
| var results = new List<ScriptResult>(); | ||
|
|
||
| foreach (var script in scripts) | ||
| { | ||
| // The `env` script is special so if it's not explicitly declared we act like it was | ||
| if (projectScripts.ContainsKey(script) || string.Equals(script, "env", StringComparison.OrdinalIgnoreCase)) | ||
| if (projectScripts.Contains(script) || string.Equals(script, "env", StringComparison.Ordinal)) |
There was a problem hiding this comment.
If you expose the key comparer of projectScripts, you can use that same comparer to compare script with "env" as a way to ensure that you use the same behavior in both places. If env is special and must always be a special ordinal comparison, I think it should be a dedicated (static) method with a descriptive name and maybe a description regarding why it exists. Similar code is found in CommandGroupRunner too.
xt0rted
force-pushed
the
script-loading
branch
from
9df2abc to
acd8f8d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When wildcard matching was added it became important for the order of script execution to match the order they're in the config file. This change should make sure that's the case. It also removes the case insensitive matching we were doing since it's been kind of buggy and NPM doesn't do it so people are already used to that behavior.
Closes #92