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 parser option to getSchema #26

Merged
merged 4 commits into from Nov 21, 2023
Merged

Conversation

maxh
Copy link
Contributor

@maxh maxh commented Nov 19, 2023

I finally got around to hacking on using location data in prisma-lint!

loop-payments/prisma-lint#84

Everything works great when testing in the prisma-lint repository, but I didn't get location data when running the linter on a schema in a different repository (backend) that uses prisma-lint. I think this is because prisma-ast was looking for a config file in backend.

I wasn't sure how to get the location node configuration for prisma-ast to be set by prisma-lint without requiring backend to also have to configure prisma-ast, which felt like too much to ask for users of prisma-lint.

This PR allows prisma-lint to request location information on demand from prisma-ast, regardless of the config.

I recommend reviewing with the hide whitespace option enabled:

image

Copy link
Owner

@MrLeebo MrLeebo left a comment

Choose a reason for hiding this comment

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

This approach can work, but I'd like to tweak your implementation a little bit in order to prevent the library from constructing a new parser for each execution. That can negatively impact performance for bulk operations. Instead, we can change the getSchema config options to receive the parser instance itself. Then prisma-lint or any other library can expose its own parser and pass it into getSchema.

Example:

import { PrismaParser, getSchema } from '@mrleebo/prisma-ast'
const lintParser = new PrismaParser({ nodeLocationTracking: "full" })
getSchema(source, { parser: lintParser })

I made some code suggestions with the core ideas of the change. You'll also need to add PrismaParser to the index.ts exports

export { PrismaParser } from './parser';

The other changes you made like the visitor factory make sense and we can keep that.

src/getSchema.ts Outdated Show resolved Hide resolved
src/parser.ts Outdated Show resolved Hide resolved
data: T,
...tokens: IToken[]
): T {
if (parser.config.nodeLocationTracking === 'none') return data;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the visitor now uses the parser's config instead of receiving its own.

@maxh
Copy link
Contributor Author

maxh commented Nov 21, 2023

Thanks for the feedback. I've iterated in that direction -- let me know what you think.

UPDATE:

I confirmed this approach works from prisma-lint side: https://github.com/loop-payments/prisma-lint/pull/239/files

@MrLeebo MrLeebo merged commit 7dc2b97 into MrLeebo:main Nov 21, 2023
@MrLeebo MrLeebo changed the title Add configOverride parameter to getSchema Add parser option to getSchema Nov 21, 2023
@MrLeebo
Copy link
Owner

MrLeebo commented Nov 21, 2023

@maxh this feature should be available via prisma-ast v0.8.0

@maxh maxh deleted the maxh/allow-dynamic-config branch November 21, 2023 16:49
maxh added a commit to loop-payments/prisma-lint that referenced this pull request Nov 24, 2023
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