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

Update types for upcoming changes #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bmeck
Copy link

@bmeck bmeck commented May 17, 2023

No description provided.

{
type: 'object',
properties: {
action: {
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to add more properties to this?

*/

/**
* @typedef {boolean | SocketIssueRuleObject} SocketIssueRuleValue
Copy link
Member

Choose a reason for hiding this comment

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

A few thoughts after mulling over it. Curious what your thoughts are:

Did you consider something more like

Suggested change
* @typedef {boolean | SocketIssueRuleObject} SocketIssueRuleValue
* @typedef {boolean | 'defer' | 'error' | 'warn' | 'ignore'} SocketIssueRuleValue

Just thinking

issueRules:
  - didYouMean: 'warn'
  - installScript: 'error'

Is easier to remember than

  - didYouMean: 
       action: 'warn'
  - installScript: 
       action: 'error'

If we have plans for additional keys, maybe the object makes sense. Do we have an idea what those would be and how they would be used?

The other thought I had was what was the purpose of the defer rule action? error and warn take the place of true, ignore is a synonym for false. But omitting a rule is implicitly a defer. Does it enable some kind of new functionality or just serve the role of explicitly defining that behavior?

Copy link
Author

Choose a reason for hiding this comment

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

We definitely should look at telemetry and role based prompting here which is why I'm not keen on a string, if we do a string now it would be more breaking in the future. defer is to allow explicit saving of that so that the behavior is recorded and not changed. Some UI toggling for example of true/false with a checkbox cannot represent this currently and has an implicit (very hard to understand) 3rd state that is like defer going on right now.

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