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

Allow for array modifications, add inPlace formatting option. #35

Merged
merged 2 commits into from
Jun 29, 2020

Conversation

mbullington
Copy link
Contributor

Hello!

  • Allow for array modifications, with proper bounds checking.
  • Add inPlace option for formatting options (disabled by default), which essentially is a shortcut past withFormatting. This can be helpful for very large data files where performance is more important than file formatting.

Thanks again for this great library!

@aeschli
Copy link
Contributor

aeschli commented Jun 9, 2020

Thanks.
We have the same code in vscode and insertions got implemented there
microsoft/vscode@fa2eff9#diff-0a43f142588962451d1bba0023dff15d

I want to bring the two together. If you are interested doing so that would be great.

@aeschli aeschli added this to the June 2020 milestone Jun 9, 2020
@mbullington
Copy link
Contributor Author

Sure, I'm happy to help unify the two but would like some feedback on how to proceed.

In VSCode, the commit you mentioned supports adding an element to the array at a specific index. This PR, when given an already existing index, will replace that index's content.

Perhaps a replacement policy option to support both use cases?

@aeschli
Copy link
Contributor

aeschli commented Jun 10, 2020

Agree, we need something to distinguish the cases. Replace should be the default.
Possibilities

  • allow to use fractions: [1.5] to insert between 1 and two
  • have a ModificationOptions.isInsert

@mbullington
Copy link
Contributor Author

Just updated to include a ModificationOptions.isArrayInsertion option that is false by default, and also co-oped the unit tests from microsoft/vscode@fa2eff9#diff-0a43f142588962451d1bba0023dff15d. My only concern is that if VSCode syncs the code from this repo (or if they have diverged?), the default of replacement is different and may cause issues.

@stevenguh
Copy link

stevenguh commented Jun 28, 2020

Thank you guys. I happen to need this feature. I am wondering is there any update on this?

@aeschli aeschli merged commit b905205 into microsoft:master Jun 29, 2020
@aeschli
Copy link
Contributor

aeschli commented Jun 29, 2020

Thanks @mbullington !

@stevenguh
Copy link

@aeschli I don't mean to be pushy, but I am wondering is there a timeline for a new release that has this change? Thank you

@aeschli
Copy link
Contributor

aeschli commented Jul 3, 2020

Published jsonc-parser@2.3.0

Note: I removed FormattingOptions.inPlace. Instead, ModificationOptions.formattingOptions is now optional. If not set, newly inserted content will not be formatted.

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