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

Add support for cross-imports with @x #103

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Add support for cross-imports with @x #103

merged 4 commits into from
Oct 11, 2024

Conversation

illright
Copy link
Member

Closes #29

@@ -44,7 +44,7 @@ const noPublicApiSidestep = {
}

if (isSliced(dependencyLocation.layerName)) {
if (dependencyLocation.segmentName !== null) {
if (dependencyLocation.segmentName !== null && dependencyLocation.segmentName !== '@x') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't such standardized folder names be stored in some self-describing constant (that can be referenced everywhere throughout the project), rather than just a string literal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it should even be provided by feature-sliced/filesystem, but we can temporarily add it locally for the plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. I'll add it to the filesystem package

@@ -23,12 +23,20 @@ const noReservedFolderNames = {

for (const violatingFolder of findAllRecursively(
child,
(entry) => entry.type === 'folder' && conventionalSegmentNames.includes(basename(entry.path)),
(entry) => entry.type === 'folder' && conventionalSegmentNames.concat('@x').includes(basename(entry.path)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as no-public-api-sidestep

@daniilsapa
Copy link
Collaborator

@illright

Excellent job!

It's good that you decided to add a separate message for the @x case in no-reserved-folder-names that describes the issue better, it looks reasonable.
Also, it’s great that it checks the correctness of cross-imports (imported by the entity for which it is exactly intended
). It looks like you've covered all the rules that needed fixes, and more subtle edge cases too.

There was just a single minor call out from me

Copy link
Collaborator

@daniilsapa daniilsapa left a comment

Choose a reason for hiding this comment

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

Perfect, all necessary changes have been made!

@illright illright merged commit aa81a28 into master Oct 11, 2024
7 checks passed
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.

Support cross-imports with @x
2 participants