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

The groupBy feature #146

Merged

Conversation

AlexandrHoroshih
Copy link
Contributor

@AlexandrHoroshih AlexandrHoroshih commented Mar 4, 2024

Summary

This PR adds the first part for "grouped graph" feature, as discussed in #125 :

  • Optional groupBy configuration is added - it is a function of (path: string) => string | undefined type
  • If groupBy is provided, then groupedGraph will be emitted on getStructure call

Example

const instance = await skott({
 groupBy: (path) => {
          if (path.includes("src/core")) return "core";
          if (path.includes("src/features/feature-a")) return "feature-a";
          
          return undefined;
      }
  }
});

const {groupedGraph} = instance.getStructure();

groupedGraph.core // { id: "core", adjacentTo, body: { size, files, ... } }
groupedGraph["feature-a"] // { id: "feature-a", adjacentTo, body: { size, files, ... } }

Implementation

As was discussed in the issue, there is a single groupBy function, which assigns module to a group based on its node.id (which is the path of the module location).

Under the hood it is the separate graph, which is built in the moment of getStructure call, using the original graph of project structure as a source of truth.

Testing

Integration tests in the integration/api.spec.ts

Impacted documentation

The changeset is generated and the JSDoc comment is added to the groupBy field, also documentation is added into skott/README.md

  • Changesets were generated using pnpm changeset at the root of the workspace, affected packages are being bumped (either patch/minor) and a clear description for each of the affected packages was added.

Copy link
Owner

@antoine-coulon antoine-coulon left a comment

Choose a reason for hiding this comment

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

Great work @AlexandrHoroshih, it seems to work pretty well!

I published few comments regarding the PR, but overall looks good 👍

packages/skott/src/skott.ts Outdated Show resolved Hide resolved
packages/skott/src/skott.ts Outdated Show resolved Hide resolved
packages/skott/src/skott.ts Outdated Show resolved Hide resolved
packages/skott/src/skott.ts Outdated Show resolved Hide resolved
Copy link
Owner

@antoine-coulon antoine-coulon left a comment

Choose a reason for hiding this comment

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

LGTM

@antoine-coulon antoine-coulon merged commit 5ae16c9 into antoine-coulon:main Mar 7, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Mar 7, 2024
@antoine-coulon
Copy link
Owner

@all-contributors please add @AlexandrHoroshih for code

Copy link
Contributor

@antoine-coulon

I've put up a pull request to add @AlexandrHoroshih! 🎉

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.

None yet

2 participants