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

Create table language 54 #188

Closed
wants to merge 16 commits into from

Conversation

AzaniaBG
Copy link
Member

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.

@AzaniaBG AzaniaBG requested a review from fyliu August 17, 2023 17:25
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

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

Thanks for contributing the PR. I think the code looks great!

There's just a couple of chores that's missing.

  1. It looks like you don't have pre-commit working. It should have fixed some of the whitespace and import ordering issues automatically. I'm able to tell because it would've sorted the import lines alphabetically in files like models.py.
  2. You should always rebase your changes to the latest hackforla/main branch once you have it working and before submitting the PR. That way, if some other changes got merged, you’ll notice any conflicts and be able to fix them. The general thinking is whatever got merged into the upstream repo is working, and you’re trying to merge your working changes into it, but you have to make sure your changes can work with the latest code that’s there.
    In general software development, sometimes the changes are in different files or different parts of the file so that they don’t conflict. But for us doing these “create table…” issues, the changes are all very similar and it’s pretty much going to conflict all the time.

A few notes half for myself:

  • I should say that I'm noticing that the lint script doesn't seem to be doing what it should. I will need to debug that. The problem was that I didn't have the docker container running in the background. The lint script should have called black to do the formatting before calling flake8. I will commit the change to the main repo.
  • I will also try to do the rebase for this PR and add the steps to some documentation page for future reference.
  • There's also the issue of non-ideal situation where the pre-commit needs installation whereas the rest of the project just runs from docker. We'll have create an issue to put it into docker.

@fyliu
Copy link
Member

fyliu commented Aug 23, 2023

@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
Copy link
Member Author

@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: #190. I'd like to try the rebase myself, thanks.

@AzaniaBG
Copy link
Member Author

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.

brdeleon and others added 7 commits August 26, 2023 09:07
* feat: add model: technology

* feat: register admin: technology

* feat: add endpoints: technology

* Lint warnings and code clean up. Removed unnecessary migration files. Corrected admin name.

* "Lint and code cleanup. Removing previous technology migration. Admin.py name update."

* Modified migrations for technology table.
@AzaniaBG AzaniaBG requested a review from fyliu August 26, 2023 17:54
@AzaniaBG
Copy link
Member Author

AzaniaBG commented Aug 26, 2023

@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.

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

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

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.

@AzaniaBG
Copy link
Member Author

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
Copy link
Member

fyliu commented Aug 30, 2023

@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
Copy link
Member

fyliu commented Sep 12, 2023

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

@fyliu fyliu closed this Sep 12, 2023
@AzaniaBG AzaniaBG deleted the create-table-language-54 branch September 17, 2023 17:19
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.

Create Table: language 😄
3 participants