Skip to content

Conversation

@auduchinok
Copy link
Member

@auduchinok auduchinok commented Dec 18, 2018

Storing kind removes some workarounds used by FCS clients for checking whether a module is anonymous and whether a namespace is global.

Adds the following type to replace isModule: bool fields.

[<Struct>]
type SynModuleOrNamespaceKind =
    | NamedModule
    | AnonModule
    | DeclaredNamespace
    | GlobalNamespace

@auduchinok auduchinok changed the title [WIP] Replace isModule with ModuleOrNamespaceKind in SymModuleOrNamespace [WIP] Replace isModule with ModuleOrNamespaceKind in SynModuleOrNamespace Dec 18, 2018
@auduchinok
Copy link
Member Author

auduchinok commented Dec 18, 2018

I updated structure tests. Despite having the following comment implicit modules ranges were returned in structure service:

// do not return range for top level implicit module in scripts

Checking AnonModule kind only makes Structure service not return these ranges for source files as well. Would this behaviour be expected or do we want to get folding ranges for implicit modules in source files and not get them in scripts? I'd prefer the same behaviour of not getting these ranges.

I also added cases for explicit module and namespace cases there.


// do not return range for top level implicit module in scripts
if isModule && not isScript then
if kind <> AnonModule then
Copy link
Contributor

Choose a reason for hiding this comment

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

The check should probably be kind = NamedModule since we don't want to include namespaces?

I think you might be right that we should have the same behavior for anon modules in scripts and non-scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind having an option to fold a namespace but it would require fixing folding ranges in Structure provider which is better to have in a separate PR. Thanks, I'll replace the check.
screenshot 2018-12-19 at 19 13 18
screenshot 2018-12-19 at 19 13 25

Copy link
Member Author

@auduchinok auduchinok Dec 19, 2018

Choose a reason for hiding this comment

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

I don't see a reason to fold a top level module, though, as we can't have other modules/namespaces below it.
screenshot 2018-12-19 at 19 27 07

@TIHan
Copy link
Contributor

TIHan commented Dec 18, 2018

I like the PR. It makes sense that we have a SynModuleOrNamespaceKind to represent the different namespaces and modules that we have.

@auduchinok auduchinok changed the title [WIP] Replace isModule with ModuleOrNamespaceKind in SynModuleOrNamespace Replace isModule with ModuleOrNamespaceKind in SynModuleOrNamespace Dec 19, 2018
@auduchinok
Copy link
Member Author

It seems to be ready.
cc @nojaf @jindraivanek @nosami @Krzysztof-Cieslak

module MyModule =
()
"""
=> [ (1, 0, 4, 0), (1, 0, 4, 0)
Copy link
Contributor

@TIHan TIHan Dec 20, 2018

Choose a reason for hiding this comment

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

Looking at this again, why are some of these ranges removed from the tests? It looks like it was giving you the range of the whole module itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's removing ranges of anon top level modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yes that makes sense. That's just an inner module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's nice

@TIHan TIHan merged commit e2902ee into dotnet:master Dec 22, 2018
@TIHan
Copy link
Contributor

TIHan commented Dec 22, 2018

Thank you

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.

4 participants