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

Allow multiple spaces in PRISMA_DIRECT_URL_REGEXP #8001

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

dennemark
Copy link
Contributor

Allows multiple spaces between directUrl = env, since some code formatters add these to schema.prisma.

Allows multiple spaces between directUrl = env, since some code formatters add these to schema.prisma.
@thedavidprice thedavidprice requested a review from dthyresson April 3, 2023 18:25
@thedavidprice
Copy link
Contributor

@dennemark the regex change looks fine. It just seems strange to me there could be multiple whitespaces. re: "since some code formatters add these to schema.prisma." Can you paste an example and give a bit more background?

@dthyresson Could I loop you in on this one? I'm likely being too cautious on this one. Just didn't want to open things up too wide and someone end up with an edge case bug.

@dthyresson dthyresson self-assigned this Apr 3, 2023
@dennemark
Copy link
Contributor Author

image
The official Prisma VSCode extension does this kind of formatting. https://marketplace.visualstudio.com/items?itemName=Prisma.prisma

format

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

@dennemark Thanks for this improvement. As I understand it, the regex needs to handle the case where you've added a directUrl and Prisma prettify-ing has added spaces to align the content. And thus, formatter with the spaces, the regex won't match.

Makes sense.

Could we add this scenario to the tests so there is a test for:

  • no directUrl
  • formatted, unaligned
  • formatted, aligned with spaces

That way we cover the cases and won't have a regressions.

Thanks!

@dennemark
Copy link
Contributor Author

  • no directUrl
  • formatted, unaligned
  • formatted, aligned with spaces

I have added the formatted test, the other ones are already existing. They take their schema from fixtures. Those fixtures are actually also formatted, but they got along with one space test, since they did not include the shadowDatabaseUrl. I added a test case where I did not import from fixtures, but provide a multi-line string. Works out.
I did not want to export the Regex, so I think this additional test should be ok?!

@dthyresson dthyresson added the release:chore This PR is a chore (means nothing for users) label Apr 5, 2023
@dennemark
Copy link
Contributor Author

dennemark commented Apr 20, 2023

is it possible to merge? :)

@dthyresson dthyresson enabled auto-merge (squash) April 25, 2023 20:14
@dthyresson dthyresson merged commit d81aa49 into redwoodjs:main Apr 25, 2023
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Apr 25, 2023
@jtoar jtoar modified the milestones: next-release, v5.0.0 Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants