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

Improve clarity of schema drift summary explanation #4666

Closed
wants to merge 3 commits into from

Conversation

levindixon
Copy link

This PR updates the DRIFT_DETECTED_MESSAGE with the goal of improving the clarity of the summary listed below it.

This change was made because I found the phrasing of the last line somewhat confusing which lead me to misinterpret the summary of changes as "These are the changes needed to be made to the database in order to have the database schema align with the schema Prisma is expecting it to have".

Before:

Drift detected: Your database schema is not in sync with your migration history.

The following is a summary of the differences between the expected database schema given your migrations files, and the actual schema of the database.

It should be understood as the set of changes to get from the expected schema to the actual schema.

[+] Added enums
  - cat

After:

Drift detected: Your database schema is not in sync with your migration history.

The following is a summary of the differences between the expected database schema given your migrations files, and the actual schema of the database.

This summary details discrepancies by listing elements found in the current database schema but missing in your Prisma schema, indicating the Prisma schema updates required for alignment.

[+] Added enums
- cat  

@levindixon levindixon requested a review from a team as a code owner January 23, 2024 00:05
@levindixon levindixon requested review from laplab and Weakky and removed request for a team January 23, 2024 00:05
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codspeed-hq bot commented Jan 23, 2024

CodSpeed Performance Report

Merging #4666 will not alter performance

Comparing levindixon:main (8adf9f5) with main (45a0eb4)

Summary

✅ 11 untouched benchmarks

@SevInf SevInf added the PR: Improvement A PR that improves existing functionality label Mar 20, 2024
@laplab laplab self-assigned this Apr 4, 2024
@laplab
Copy link
Contributor

laplab commented Apr 11, 2024

Thank you for submitting the PR, @levindixon!

The original message deliberately lacks the call to action. While it might be possible that in your usecase the Prisma schema needs to be adjusted, in other cases maybe the user is ready to adapt the database schema to match Prisma schema.

Both cases are very distinct and wrong choice can result in data loss, so we need to be extra careful in what we suggest to the user. Your change adds the call to action, which might further confuse users with the second usecase.

Thus, I am afraid we cannot accept your contribution in its current form. If you believe that the message can be improved without adding the call to action, we will be happy to review it.

@laplab
Copy link
Contributor

laplab commented Apr 22, 2024

@levindixon I am going to close this PR for now. Please feel free to either reopen this PR when you have some time, or open a new one.

@laplab laplab closed this Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Improvement A PR that improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants