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 or remove(if appropriate) incorrect defaults. #77

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Jan 19, 2024

Closes #75 because the root cause of it was an incorrect assumption made from incorrect defaults.

@noppej noppej self-requested a review January 19, 2024 18:42
Copy link
Contributor

@noppej noppej left a comment

Choose a reason for hiding this comment

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

@bugadani Is this a work in progress? I cannot see where in the currently committed code the defaults have been changed.

Also, I wonder if removing the defaults might do more harm than good? Would it not be a better user experience to harmonize the defaults with the probe-rs implementation. That way, launch.json will hint at the correct default.

Finally, if we change the schema, it would be really nice if we can cleanup the appropriate references in the docs. If that is too much to ask, please open an issue against https://github.com/probe-rs/webpage, and I'd be happy to add it to my list.

@Yatekii
Copy link
Member

Yatekii commented Jan 19, 2024

He just synchronized the VSCode config schema with the actual default values we use :)

@noppej
Copy link
Contributor

noppej commented Jan 19, 2024

Github is so strange. I looked at the changes just before typing that comment, and there were no changes to the default in package.json ... just the dependency updates.

I looked again, and they are all there.

@noppej
Copy link
Contributor

noppej commented Jan 19, 2024

The code changes looks fine, and I'm happy to approve. However, if we release this PR, the docs will contradict the product.

@noppej noppej changed the title Remove incorrect defaults Update or remove(if appropriate) incorrect defaults. Jan 19, 2024
@Yatekii
Copy link
Member

Yatekii commented Jan 19, 2024

Tbh, this is the docs. If you configure stuff you look at the autocompletion of vscode :)

@noppej
Copy link
Contributor

noppej commented Jan 19, 2024

I'd be OK if we decide to rip relevant pieces where contradictions exist, from the docs? It makes more sense now that intellisense actually works (courtesy of @bugadani efforts ☺️)

@bugadani
Copy link
Contributor Author

For some cases the defaults made no sense to me. For example, the SVD path - if it's not set, probe-rs just doesn't use it instead of defaulting to whatever the extension specified. Program binary is required, so a default again makes little sense to me. Forgot to sack npx, sorry about that

Keeping the minor version in synch with probe-rs. Also, this will trigger a publish to the marketplace.
Copy link
Contributor

@noppej noppej left a comment

Choose a reason for hiding this comment

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

Thanks @bugadani ... having "working" intellisense, that gives the "correct" guidance is valuable for usability.

@noppej noppej merged commit 6e5b9ac into probe-rs:master Jan 19, 2024
3 checks passed
@bugadani bugadani deleted the defaults branch January 19, 2024 19:19
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.

Display the flashing progress in some form
3 participants