-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Contributed tasks aren't added to recent #94549
Conversation
Background info: The fix is to save the task as recently used with the correct custom key instead of the incorrect contributed key. |
tasks: [customizations] | ||
}, TaskRunSource.System, custom, customized, TaskConfig.TaskConfigSource.TasksJson, true); | ||
for (const configuration in customized) { | ||
key = customized[configuration].getRecentlyUsedKey()!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the key a random key from the customized hash table ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key is from here
public getRecentlyUsedKey(): string | undefined { |
I start with a
ContributedTask
key, but I'm storing a CustomTask
, so when I re-create the task later it will have a CustomTask
key. To avoid this, I convert the ContributedTask
into a CustomTask
then use that key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I actually mean is the following: how can this piece of code be deterministic given that we loop over the keys in a hash table. There is no order associated with this. Or does it simply not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be only one key in the hash table, since only one task is fed in. I will make that clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. That explains it to me. Thanks.
tasks: [customizations] | ||
}, TaskRunSource.System, custom, customized, TaskConfig.TaskConfigSource.TasksJson, true); | ||
for (const configuration in customized) { | ||
// There should only be one configuration in customized, but check that it matches the original task just to be sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens now if we do have a programming error somewhere and the keys are not the same. The we use the key from the beginning which to my understanding would be wrong. So shouldn't we simply assert this or ignore the task if something goes wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is effectively ignored later when the recents are read. However it would be better to ignore earlier (here) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. That makes sense now to me. Are you planning to do any changes or do we go with the ignore later approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change for ignoring here since it is better not to defer the problem.
Thank you for the review! |
This PR fixes #94547