search

hackforla / peopledepot

A project to setup a datastore for people and projects at HackforLA. The link below takes you to the code documentation
https://hackforla.github.io/peopledepot/
GNU General Public License v2.0
5 stars 24 forks source link

Create table language 54 #188

Closed AzaniaBG closed 10 months ago

AzaniaBG commented 10 months ago

Fixes #54 The how-to Guide indicates the read-only field is updated_at so I added that field instead of using the ERD's updated_on field.

fyliu commented 10 months ago

@AzaniaBG I created issues to create the necessary docker config and docs so that you can help yourself. I can also show you how I would do this during the meeting.

AzaniaBG commented 10 months ago

@AzaniaBG I created issues to create the necessary docker config and docs so that you can help yourself. I can also show you how I would do this during the meeting.

Thanks, @fyliu ~ for some reason I don't see steps to dockerize pre-commit - I just see this issue: https://github.com/hackforla/peopledepot/issues/190. I'd like to try the rebase myself, thanks.

AzaniaBG commented 10 months ago

Update: I'm still having issues fetching from upstream, so I can't run rebase the changes. I thought I fixed the issue where my local repo was pulling from the fyliu repo., but apparently not. I believe Fang and I are going to work on this at today's meeting.

AzaniaBG commented 10 months ago

@fyliu ~ we rebased/resolved conflicts from main at last Thursday's meeting. I installed pre-commit and it appears to be working. I pushed my changes and requested a re-review. Let me know if you have any questions.

AzaniaBG commented 10 months ago

Thanks for putting in the work to rebase the changes, but it looks like you might have done the rebase in the other direction, from the main branch onto the feature branch. Can you rebase from the feature branch to the main branch?

Here's the current state of your git history. Screen Shot 2023-08-30 at 12 46 32 AM

This is the way to think about it: the code in main has already gone through review and merged. You have new changes that needs to be reviewed, and they should continue from where the current main branch ends. That's where I drew the red arrow to show how your first commit should be connected to main.

The way you have it now is your changes start before main and you took the last commit from main and applied it after your changes, essentially splitting main history. I know that the final code might end up being the same, but in reordering the commits, you're making the already-reviewed commits into commits that need review, because it's possible the way you applied Brenda's commit could have changed the functionality that's there.

The thing to remember is you can rewrite your own changes but not other people's changes. It's something repo maintainers won't allow to happen. Whatever commits are already in main shouldn't be changed, and new changes should be applied after.

You should instead create a new branch at "removed updated_on from field..." and run git rebase main and go through any conflict resolution steps for each of your commits. It's a good amount to do but that's what you would have to do to keep all of your commits. That's why I suggested before to squash them into one, or merge some of the fixer commits into the feature ones. I think it might be good if I could show you how I would do the rebase to keep all the commits. I've done really long rebases before and I'm fairly comfortable with the process.

@fyliu ~ after we rebased the feature branch last Thursday, I tried to push the changes and got an error about diverged branches and “failed to push some refs…”. That's why I had to resolve conflicts again. Can I merge the changes instead of rebasing them?

fyliu commented 10 months ago

@AzaniaBG Sure. Merge and resolve conflict works. It just has to be done so that the PR shows no conflicts. We can't resolve conflicts on github itself because the migration files need to be regenerated, and also I think it's a generally accepted practice for code contributors to do this part so that the maintainers don't have to. The reason is that the contributor has a better idea how and where the new code should be added to the code base.

Sorry to get into the weeds with git for this. I can do all this if you want. There's nothing wrong with your code other than some code formatting and the migration files. The rest is just a git exercise with the rebasing. It's good to know but this PR got complicated because of not having pre-commit to format the code at each commit.

I think this PR would make a good exercise for developers wanting to play with git rebase and to try different strategies because of the complications.

fyliu commented 10 months ago

I'm going to close this one since we're going to merge #195 instead.