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

feat: add prisma format functionality #1798

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mcnaveen
Copy link

@mcnaveen mcnaveen commented Aug 9, 2024

  • This PR adds format option to the Prisma language tool when the schema.prisma file is opened.

Warning

Untested code. Looking for someone to test it locally. I'm unsure about running and building it. But I added the functionality. Hope this works or Happy to fix and Improve it.

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2024

CLA assistant check
All committers have signed the CLA.

@mcnaveen mcnaveen marked this pull request as draft August 9, 2024 15:17
@mcnaveen
Copy link
Author

mcnaveen commented Aug 9, 2024

I am looking for someone to test it locally. If you're interested. Please contribute.
Thank you. 🙏

@mcnaveen mcnaveen marked this pull request as ready for review August 13, 2024 05:14
@apolanc apolanc added this to the 5.19.0 milestone Aug 13, 2024
@apolanc apolanc requested a review from jkomyno August 13, 2024 14:26
@apolanc apolanc self-assigned this Aug 20, 2024
@apolanc
Copy link

apolanc commented Aug 20, 2024

Hello @mcnaveen,

First of all, thanks for your contribution and willingness to collaborate in making prisma better!

Have you tried using the built-in format option in VSCode? Language formatting via the Prisma’s extension formatting tool is automatically invoked by VSCode when running the “Format Document” command from the taskbar. I think that should serve the purpose to what you're trying to achieve here.

Screen.Recording.2024-08-13.at.17.44.07.mov

@mcnaveen
Copy link
Author

Hi @apolanc Thanks for giving feedback on this PR.

You're making a great point on the VSCode format option and I agree with that. But still, there is a functionality prisma format does is auto mapping the relations between models.

Let's say you have two models User and Posts where the user belongs to multiple posts/owns multiple posts. In that case, you have a userId field in the Posts table/model that refers to the Id of the User table.

Running the prisma format will make sure to reference it in the Users table if it's not already mapped. This won't work with the VSCode feature you're mentioning in the above video.

I hope you're getting my point. If it doesn't make sense. let me know, I'll record a video.

Thank you.

@apolanc
Copy link

apolanc commented Aug 21, 2024

@mcnaveen thanks for the answer. Please record a short video to showcase what you had in mind here :)

I'll keep this contribution PR open until we get the capacity to review it.

Thanks once again!

@mcnaveen
Copy link
Author

Hi @apolanc I didn't realize it works with the VSCode format option.

prisma-format.mp4

@apolanc
Copy link

apolanc commented Aug 21, 2024

Hi @apolanc I didn't realize it works with the VSCode format option.

Help me here a bit, would you still see this PR as beneficial or do you realize it's not necessary anymore? :)

@apolanc
Copy link

apolanc commented Aug 23, 2024

@mcnaveen thoughts here? if none, I'll close this PR to not keep it hanging.

@mcnaveen
Copy link
Author

Hi @apolanc Yes, it's a nice option to have in terms of DX. It can be merged!

@apolanc apolanc removed their assignment Aug 23, 2024
@apolanc apolanc removed the request for review from jkomyno August 23, 2024 11:02
@apolanc
Copy link

apolanc commented Aug 26, 2024

Thanks for the answer. I'll add this in our list of community contributions and an Engineer should be able to review and evaluate the solution. We will then decide what will happen in the PR or if we need anything from you, we'll let you know.

Thanks once again for contributing!

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