-
Notifications
You must be signed in to change notification settings - Fork 12
fix: section_id column to meeting_id for clarity #260
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
- This does not require a documentation change, since it changes an internal implementation detail.
- Currently, this PR does nothing because there is no migration. More information here. You need to take further steps in order for this PR to be ready to merge.
|
Sorry about the incomplete PR. Can you please check this new one. Thanks! |
|
Was looking through the PR backlog and found this. In the future, if a reviewer requests changes on your PR, and you address these changes, you should then re-request a review (with the re-request review flow in GitHub) as a courtesy, otherwise they may not be aware of your work. Any outstanding requested changes will prevent the PR from being merged. This includes requests for changes that have been resolved but have not been reviewed and approved. I'll review this now since I'm here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. The idea looks good, but you'll have to regenerate the migration file because a migration sharing ID 19 has been merged into the codebase since this PR was opened. Migrations are sequential so this ID overlap must be resolved.
Fixing this is a little troublesome:
- Rectify your branch so that the
dbfolder exactly matches what's currently on ourmainbranch, with no added or deleted files. - Make the change to the column name again and generate a new migration. Make sure you name the migration per my comments.
- Commit the result.
This is a somewhat annoying artefact of the way Drizzle works. I'm happy to guide you through this (contact me/us on Discord) if you need help, because I see this as an undue burden on first-time contributors.
|
Sorry for not notifying your about the updated PR. I'm glad you were able to find it and thank you for all the helpful comments. I created a new migrate ID. Let me know if everything looks good. Thank you! |
laggycomputer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration stuff looks fine now. Looks like some other artefacts got mixed in. Clear those out and we should be good.
|
Thanks for pointing those out. I've corrected them now. |
laggycomputer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this is good now. Thanks for the patch!
sanskarm7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey nathan,
thanks for your hard work over these last few weeks! just tested the changes and all looks good. well done!
last thing, and it's such a minor nitpick i almost didn't see it - could you please rerun the migration with the name rename_section_id_to_meeting_id rather than rename-section-id-to-meeting-id ?
it would just make it more consistent with the other migrations haha.
thanks again for your work! looking forward to getting this merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconding the above, also please follow the Conventional Commits spec in the PR title since this will become the commit message.
|
Thanks for all the thorough reviews and the patience. Appreciate your help. |
sanskarm7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yahooo lgtm!!
good work nathan!
The column references websocSectionMeeting.id (a meeting ID), but was incorrectly named section_id.
Related Issue
#249
Motivation and Context
The previous naming was confusing and misleading for readers. Now it is more intuitive.
How Has This Been Tested?
Only column name was changed. It had no effect on the code, so the code runs fine just like before.
Screenshots (if appropriate):
Types of changes
Checklist: