close
Skip to content

Add default endianness setting#215

Merged
lramos15 merged 5 commits into
microsoft:mainfrom
natecraddock:endianness-preference
Mar 19, 2021
Merged

Add default endianness setting#215
lramos15 merged 5 commits into
microsoft:mainfrom
natecraddock:endianness-preference

Conversation

@natecraddock

Copy link
Copy Markdown
Contributor

This adds a setting to set the endianness selected in the data inspector by default on load. This is useful when working on a large number of big-endian files so the data inspector dropdown doesn't need to be changed each time a new file is opened.

Adds a configuration property to configure the endianness selected in
the data inspector on load.
@ghost

ghost commented Mar 4, 2021

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@natecraddock

natecraddock commented Mar 4, 2021

Copy link
Copy Markdown
Contributor Author

The contributing guidelines weren't clear if all pull requests are required to have an associated issue. If that's the case let me know and I will add one.

Also, this is my first time editing a VSCode extension, so hopefully it follows general standards.

@lramos15

Copy link
Copy Markdown
Member

Hi @natecraddock. I'll take a look at this this week. Do you mind also updating the README.md file with the new setting, similar to how the other setting is listed

@lramos15 lramos15 self-requested a review March 15, 2021 13:49
@natecraddock

Copy link
Copy Markdown
Contributor Author

Thanks! I've updated the readme

@lramos15

Copy link
Copy Markdown
Member

@natecraddock Thanks for the quick update and I'm probably going to sound like a broken record. I'm planning to review your PR tomorrow do you mind removing the setting from the readme (Since @Tyriar pointed out it's duplicated in the contributions section and he just did a bunch of README cleanup) and then solve the merge conflicts :)

@Tyriar Tyriar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using a setting here, would it make more sense to just remember the previous selection made and apply that to new files?

Pros:

  • No additional setting to know about/configure

Cons:

  • If you frequently work with both, you'd need to switch back to little

@lramos15

lramos15 commented Mar 19, 2021

Copy link
Copy Markdown
Member

If we were to "remember" the setting we would have to have some sort of hex editor config that we right as I'm not sure how the state can be preserved.

@natecraddock

Copy link
Copy Markdown
Contributor Author

@Tyriar I think a setting is better. In my case, I have a project which uses entirely big endian formatted data, but others that use little endian. The setting makes it easy to configure per-workspace how to interpret the data by default.

Also, with only 3 settings, it's not like there is an overload of too many settings here.

@lramos15 lramos15 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thanks! I also agree a setting feels cleaner here as the idea of remembering what you picked for previous files would add complexity and might confuse the user.

@lramos15 lramos15 merged commit 09a1b61 into microsoft:main Mar 19, 2021
@Tyriar

Tyriar commented Mar 19, 2021

Copy link
Copy Markdown
Contributor

Sounds good 🙂

@lramos15 fyi there's apis in the exthost that let you store workspace or user-level info like this

@lramos15

Copy link
Copy Markdown
Member

Ooh that's neat. Maybe something to look into in the future 🙂

@lramos15 lramos15 added this to the March 2021 milestone Mar 22, 2021
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