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

Use "file local types" instead of "file types" #62514

Merged
merged 3 commits into from
Jul 13, 2022

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jul 8, 2022

related to #62254 and #60819

We use "file local types" to refer to the language concept, e.g. in INamedTypeSymbol.IsFileLocal. When we are only asking whether a declaration literally has the file modifier we still say IsFile, e.g. in SyntaxGenerator.

{
if (!possibleFileType.IsFile)
if (!possibleFileLocalType.IsFile)
Copy link
Member

Choose a reason for hiding this comment

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

.IsFile

Is there a missing rename here?

internal bool IsFile => HasFlag(DeclarationModifiers.File);

I see the implementation is about the modifier, but I think the API itself is meant to be about the language concept?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to IsFileLocal.

@RikkiGibson RikkiGibson marked this pull request as ready for review July 12, 2022 17:51
@RikkiGibson RikkiGibson requested review from a team as code owners July 12, 2022 17:51
@jcouv jcouv self-assigned this Jul 12, 2022
@@ -122,6 +122,6 @@ public override string MetadataName

public bool IsSerializable => false;

public bool IsFile => Modifiers.IsFile;
public bool IsFileLocal => Modifiers.IsFile;
Copy link
Member

Choose a reason for hiding this comment

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

IsFileLocal

I'm having a slight hesitation, whether this should be IsFileLocalType, like IsUnmanagedType, IsRefLikeType or IsGenericType above...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly about this here. It's an internal API so don't think there's a rush to make it happen.

Copy link
Member

@Youssef1313 Youssef1313 Jul 12, 2022

Choose a reason for hiding this comment

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

@RikkiGibson But that applies to the public INamedTypeSymbol.IsFileLocal property as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point (: I missed that this was an implementation of INamedTypeSymbol.IsFileLocal.

IMO, I think IsFileLocal is better because we anticipate generalizing the file-local concept to non-type members. If this happens, we might end up introducing API like ISymbol.IsFileLocal and then the INamedTypeSymbol.IsFileLocal would shadow it.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I could have placed the comment in a better location ;-)

we might end up introducing API like ISymbol.IsFileLocal and then the INamedTypeSymbol.IsFileLocal would shadow it.

Makes sense. Thanks

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@RikkiGibson RikkiGibson merged commit 117a3ed into dotnet:main Jul 13, 2022
@RikkiGibson RikkiGibson deleted the rename-file-local-types branch July 13, 2022 19:32
@ghost ghost added this to the Next milestone Jul 13, 2022
adamperlin pushed a commit to adamperlin/roslyn that referenced this pull request Jul 19, 2022
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants