New insert snippet option to exclude extra indentation

[rahmaniaam]

Issue #, if available: N/A

Description of changes:
By default, when item is inserted, ace will modify snippet and add indentation to match previous code line. However some tooling may already include the proper indent format, thus creating improperly indented code when inserted. This change adds the option to exclude those extra indentations.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov]

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.10 🎉

Comparison is base (b6799c1) 86.84% compared to head (a3693aa) 86.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5155      +/-   ##
==========================================
+ Coverage   86.84%   86.94%   +0.10%     
==========================================
  Files         558      560       +2     
  Lines       43734    44232     +498     
  Branches     6793     6854      +61     
==========================================
+ Hits        37980    38457     +477     
- Misses       5754     5775      +21     

Flag	Coverage Δ
unittests	86.94% <91.66%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/snippets.js	87.01% <81.81%> (-0.14%)	⬇️
src/autocomplete.js	79.01% <100.00%> (ø)
src/snippets_test.js	99.55% <100.00%> (+0.02%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[oykuyilmaz]

can we make this feature flag kind of removeExtraIndent a config option in autocomplete or similar instead of passing it through functions?

[aprex]

it needs to be passed per invocation to choose if indentation is needed, as snippets may or may not have indentation already

[nightwing]

Considering that

ace/src/snippets.js

Line 1037 in c7d691d

return exports.snippetManager.insertSnippet(this, content, options);

has options parameter, maybe we should make the api consistent and change

ace/src/snippets.js

Line 496 in c7d691d

this.insertSnippet = function(editor, snippetText, replaceRange, removeExtraIndent) {

to accept options: {indent: false, range: undefined}?

removeExtraIndent is not a very good name, because it doesn't remove anything simply prevents adding.

[rahmaniaam]

Considering that

ace/src/snippets.js

Line 1037 in c7d691d

return exports.snippetManager.insertSnippet(this, content, options);

has options parameter, maybe we should make the api consistent and change

ace/src/snippets.js

Line 496 in c7d691d

this.insertSnippet = function(editor, snippetText, replaceRange, removeExtraIndent) {

to accept options: {indent: false, range: undefined}?

Missed that piece, yeah that sounds better instead of adding more parameters. I'll update it.

removeExtraIndent is not a very good name, because it doesn't remove anything simply prevents adding.

Good point, excludeExtraIndent is probably a better wording for it

[nightwing]

Looks good, Thank you.
I still think {indent: false} is better wording, because it is not really extra indentation, it is not there at start to be excluded, and to a human one would just say do not indent. replaceRange is also unnecessarily wrong, but then i tend to err on the side of shorter names, so i'll let other decide.

[rahmaniaam]

I still think {indent: false} is better wording, because it is not really extra indentation, it is not there at start to be excluded, and to a human one would just say do not indent.

Personally I prefer the current one just because by default we add those indentations and it removes the need to explicitly type default arguments like options={indent: true} into our apis

replaceRange is also unnecessarily wrong, but then i tend to err on the side of shorter names, so i'll let other decide.

I think changing to range is okay here, I'll update it if no one else has a problem with it, along with a debug line a forgot to erase