oleg-shilo / codemap.vscode

Code map (syntax tree) of the active document
MIT License
84 stars 28 forks source link

Add option to disable default mapper, add base mapper, support \n in generic_mapper #39

Closed adamerose closed 2 years ago

adamerose commented 4 years ago

https://github.com/oleg-shilo/codemap.vscode/issues/38 https://github.com/oleg-shilo/codemap.vscode/issues/37

adamerose commented 4 years ago

Let me know if you need any changes to accept this, or just close it if you think the features wouldn't be useful to others.

FYI this is the use case I was working towards with my 2 issues and these changes image

oleg-shilo commented 4 years ago

Thank you.

I see what you are trying to do. All good. The only thing I would change is the naming of the config values.

Thus the term "defaults" semantically isn't a great choice for referring to the all mappers that included in the CodeMap distro and created by the user. In fact "base mapper" seems to be even a better candidate to be called "default". But I like "base mapper" better anyway.

So can you please change the name of the config value to something like:

"codemap.useBaseMapperOnly": {
    "type": "boolean",
    "default": false,
    "description": "Disable all file type specific mappers and use base mapper only."
},

Otherwise all good.

===================

And if you do not mind a general feedback. The aggressive formatting of the contributed code makes it difficult to follow the code changes. If you have a simple way of reverting it back to default VSCode formatting for TS then you may want to use it for the code you are contributing.

But I can understand that it may be difficult on practical level.

adamerose commented 4 years ago

Thanks for the feedback.

And if you do not mind a general feedback. The aggressive formatting of the contributed code makes it difficult to follow the code changes. If you have a simple way of reverting it back to default VSCode formatting for TS then you may want to use it for the code you are contributing.

But I can understand that it may be difficult on practical level.

Undoing the formatting was tricky so I just reverted and recommitted my changes with Prettier disabled then did a force push, so it should be cleaned up now.

Thus the term "defaults" semantically isn't a great choice for referring to the all mappers that included in the CodeMap distro and created by the user. In fact "base mapper" seems to be even a better candidate to be called "default". But I like "base mapper" better anyway.

Actually my intention was for this setting to only disable the default mappers defined by CodeMap, but not the ones defined by users. I now realize my code logic was flawed. I tried correcting it to what I intended but had trouble since CodeMap defines some defaults in files like mapper_md.ts and some in package.json, and don't have time to follow though, so I'll leave that for someone else and just keep this pull to the other 2 items. I've removed the setting.

oleg-shilo commented 4 years ago

Not a problem. It will be enough if you just commit the non-prettified code. I'll do the rest.

...in files like mapper_md.ts and some in package.json...

This is what I meant when was saying "default" is s bit misleading.

This is what I'll do. I'll allow enabling/disabling mappers from the config. All of them even the built-in (embedded) mappers. These are only three: JS, TS and C#.

I will also allow disabling with a special value <none>. Thus there is no need for :

"codemap.ts": {
     "type": "string",
     "default": "<default>",
     "description": "Built-in mapper for TypeScript syntax. Use `<none>` if you want to disable it."
     },
"codemap.js": {
     "type": "string",
     "default": "<default>",
     "description": "Built-in mapper for TypeScript syntax. Use `<none>` if you want to disable it."
     },
"codemap.cs": {
     "type": "string",
     "default": "<default>",
     "description": "Built-in mapper for C# syntax. Use `<none>` if you want to disable it."
     },

This way you can disable any mapper. Thus there is actually no need for codemap.useBaseMapperOnly at all.

Meaning that only new functionality in this PR will be:

  1. Base mapper (applied for all extensions)
  2. Support for \n in regex based mappers
  3. Enabling/disabling all (built-in and user-defined) mappers via config.

You do #1-2 and I' do #3