Skip to content

Conversation

WowbaggersLiquidLunch
Copy link
Contributor

This was discussed lightly in #8 after it was merged.

Shared prefixes lead to long file names that are difficult to tell apart. We should only use prefixes that derive from parent type/module etc names without trailing "+"s when there is a naming clash. (Ideally, SwiftPM should be able to handle same-name files in different paths, but that's outside the scope of this PR.) "+" should be reserved for protocol conformances, such as "SemanticVersion+Comparable", which I believe is the convention in all other official open-source projects.

Currently each nested type definition is defined in its own file. I don't think this is necessary, and types that are closely related can be grouped in the same file. But that can be a follow-up PR.

@WowbaggersLiquidLunch

This comment has been minimized.

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Jan 12, 2022

When there is a naming clash such as ..A.M and ..B.M, which rule should we follow?

Rule 1: If they are in the same hierarchy (brother/sister node), they should be “AM” and "BM". If A.M is close to the root, they should be "M" and "BM" (Or "AM" and "M")

Rule 2: Always rename them as "AM" and "BM".

Specifically, in your PR, for "SymbolGraph.Symbol.Kind" and "SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind", you choose to rename them as "Kind.swift" and "FragmentKind.swift". So I guess you prefer the Rule 1?

@WowbaggersLiquidLunch
Copy link
Contributor Author

Specifically, in your PR, for "SymbolGraph.Symbol.Kind" and "SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind", you choose to rename them as "Kind.swift" and "FragmentKind.swift". So I guess you prefer the Rule 1?

I actually meant to apply rule 2, but only caught Relationship.Kind and Fragment.Kind, and missed Symbol.Kind. I'll update it now.

This also kind of shows the higher potential of naming clash when each nested type has its own file. Types that are very closely related should be grouped in the same file, so long as the file doesn't become too long. But this can be done in a follow-up PR imo. In general, I think it's good to follow the "Presentation of Type Definitions" section on the Standard Library Programmers Manual.

@WowbaggersLiquidLunch WowbaggersLiquidLunch force-pushed the file-name-avoid-shared-prefix branch from 562368b to c4dffe7 Compare January 12, 2022 15:17
@WowbaggersLiquidLunch

This comment has been minimized.

@WowbaggersLiquidLunch WowbaggersLiquidLunch force-pushed the file-name-avoid-shared-prefix branch from c4dffe7 to 0a7bc2a Compare January 26, 2022 18:13
@WowbaggersLiquidLunch

This comment was marked as outdated.

@WowbaggersLiquidLunch WowbaggersLiquidLunch force-pushed the file-name-avoid-shared-prefix branch from 0a7bc2a to 4ceef46 Compare February 19, 2022 07:46
@WowbaggersLiquidLunch

This comment was marked as outdated.

@WowbaggersLiquidLunch

This comment was marked as outdated.

1 similar comment
@WowbaggersLiquidLunch

This comment was marked as outdated.

@WowbaggersLiquidLunch

This comment was marked as outdated.

@WowbaggersLiquidLunch
Copy link
Contributor Author

@ethan-kusters can I request you for a review? I see you gave the final review on #8, which is related to this PR.

@ethan-kusters ethan-kusters self-requested a review February 19, 2022 15:20
Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

Hi @WowbaggersLiquidLunch! Sorry for the delay here. This looks good to me- @Kyle-Ye do you want to review here as well since you made the original organization change here?

@ethan-kusters ethan-kusters requested a review from Kyle-Ye March 4, 2022 02:01
Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

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

(Request change)The Sources/SymbolGraph/SymbolGraph folder and Sources/Symbol/Symbol folder seems not fit the rule. Did not know if it was the intended behavior. Could you explain a little? @WowbaggersLiquidLunch

For example, I think AccessControl should locate at Sources/SymbolKit/SymbolGraph/Symbol/AccessControl.swift rather Sources/SymbolKit/SymbolGraph/Symbol/Symbol/AccessControl.swift.

(Optional)Secondly, would it be a better choice if we put the parent node into its parent folder instead of its folder?

From

.
└── Sources
    └── SymbolKit
        ├── Mixin.swift
        └── SymbolGraph
            └── LineList
                ├── LineList.swift
                └── SourceRange
                    ├── Position.swift
                    └── SourceRange.swift

to

.
└── Sources
    └── SymbolKit
        ├── Mixin.swift
        └── SymbolGraph
            ├── LineList
            │   ├── SourceRange
            │   │   └── Position.swift
            │   └── SourceRange.swift
            └── LineList.swift

@WowbaggersLiquidLunch
Copy link
Contributor Author

(Request change)The Sources/SymbolGraph/SymbolGraph folder and Sources/Symbol/Symbol folder seems not fit the rule. Did not know if it was the intended behavior. Could you explain a little? @WowbaggersLiquidLunch

SymbolGraph.swiftt within SymbolGraph/ and Symbol.swift within Symbol are intended. Maybe there are better names for these 2 folders, but we can probably leave it for a follow-up PR.

For example, I think AccessControl should locate at Sources/SymbolKit/SymbolGraph/Symbol/AccessControl.swift rather Sources/SymbolKit/SymbolGraph/Symbol/Symbol/AccessControl.swift.

I didn't notice the nested Symbol/Symbol/ folders previously. I'll change it soon.

(Optional)Secondly, would it be a better choice if we put the parent node into its parent folder instead of its folder?

I think it might be better to just change either the folder or file's name. Maybe this can be a follow-up PR?

@WowbaggersLiquidLunch WowbaggersLiquidLunch force-pushed the file-name-avoid-shared-prefix branch from 4ceef46 to 1bf4565 Compare March 4, 2022 15:37
@WowbaggersLiquidLunch

This comment was marked as outdated.

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Mar 4, 2022

(Request change)The Sources/SymbolGraph/SymbolGraph folder and Sources/Symbol/Symbol folder seems not fit the rule. Did not know if it was the intended behavior. Could you explain a little? @WowbaggersLiquidLunch

SymbolGraph.swiftt within SymbolGraph/ and Symbol.swift within Symbol are intended. Maybe there are better names for these 2 folders, but we can probably leave it for a follow-up PR.

For example, I think AccessControl should locate at Sources/SymbolKit/SymbolGraph/Symbol/AccessControl.swift rather Sources/SymbolKit/SymbolGraph/Symbol/Symbol/AccessControl.swift.

I didn't notice the nested Symbol/Symbol/ folders previously. I'll change it soon.

(Optional)Secondly, would it be a better choice if we put the parent node into its parent folder instead of its folder?

I think it might be better to just change either the folder or file's name. Maybe this can be a follow-up PR?

Got it. I know SymbolGraph.swift within SymbolGraph and Symbol.swift within Symbol are intended.
But why you think SymbolGraph/SymbolGraph folder is needed.

For example AccessControl.swift is located at Sources/SymbolKit/SymbolGraph/Symbol/AccessControl.swift because its fullname is SymbolKit.SymbolGraph.Symbol.AccessControl.
So for Metadata.swift whose fullname is SymbolKit.SymbolGraph.Metadata, I think it should locate at Sources/SymbolKit/SymbolGraph/Metadata.swift rather at Sources/SymbolKit/SymbolGraph/SymbolGraph/Metadata.swift.

Sorry if I missed something, but I still did not get the point why we need the extra SymbolGraph folder. @WowbaggersLiquidLunch

Shared prefixes lead to long file names that are difficult to tell apart. We should only use prefixes that derive from parent type/module etc names without trailing "+"s. "+" should be reserved for protocol conformances, such as "SemanticVersion+Comparable".
@WowbaggersLiquidLunch WowbaggersLiquidLunch force-pushed the file-name-avoid-shared-prefix branch from 1bf4565 to cf2909b Compare March 4, 2022 16:41
@WowbaggersLiquidLunch
Copy link
Contributor Author

@Kyle-Ye you didn't miss anything. I missed the SymbolGraph/SymbolGraph/, and you're correct, it should be in just one layer of SymbolGraph. Just fixed it now.

@WowbaggersLiquidLunch
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

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

LGTM now.

@Kyle-Ye Kyle-Ye merged commit 224736d into swiftlang:main Mar 4, 2022
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