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: check_type #59 #370

Merged
merged 6 commits into from
Aug 26, 2024
Merged

Create Table: check_type #59 #370

merged 6 commits into from
Aug 26, 2024

Conversation

dmartin4820
Copy link
Member

Fixes #59

What changes did you make?

  • create a single model in Django (defining schema)
  • add seed data
    -write a test for the relationships this model will have with other models (e.g., creating a user and assigning them a set of
  • permissions on a project).
  • write an API end point
  • write API unit tests
  • CONTRIBUTING.md: add relative path starting from ./app to step 2.3.2
  • add-model-and-api-endpoints.md: add import model to views.py step

Why did you make the changes (we will use this info to test)?

  • create table for check_type, completing one task as part of Tables to add for VRMS #25
  • updated docs to clarify and/or include steps I didn't find in the docs

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals change
  • add-model-and-api-endpoints.md before:
    add_model_before

  • add-model-and-api-endpoints.md after:
    add_model_after

  • CONTRIBUTING.md before:
    contributing_before

  • CONTRIBUTING.md after:
    contributing_after

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 working through the issue so quickly and fixing documentation problems!

Everything works great. I looked through the code, built it, ran it, checked migrations, the admin site, and the API endpoints.

I found only very minor things that doesn't affect the functionality. Can you please fix them? Thanks!

@fyliu fyliu mentioned this pull request Aug 22, 2024
5 tasks
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.

Thank you for making the changes. Looks great!

@fyliu
Copy link
Member

fyliu commented Aug 26, 2024

@dmartin4820 I'm going to amend the later changes into the original commits before rebasing everything into main. I'm not sure if it's something you want to try doing yourself.

It involves splitting the last commit into 2, then using the last 3 commits to fixup the older commits. I use a tool to help do this quickly. I have it done locally but just wanted to know if you'd like to know how to do it as well.

Also, the repo is set to delete the PR branch after merging. Since you're using the main branch of your from for the PR, I'm not sure if it's aware enough to not try to delete it. The delete is supposed to be reversible if it does happen.

@dmartin4820
Copy link
Member Author

@fyliu sure, I'd be willing to try amending the commits. I'm familiar with soft resetting to a specific commit, the amending the unstaged changes to the last commit. After that, I'd need to force push to my fork

Not sure I know how to fixup the unstaged changes to specific commits, especially without a tool. But willing to learn more about it.

I don't mind if it deletes the branch of the fork or whatever it ends up doing. I can create a new one.

@fyliu
Copy link
Member

fyliu commented Aug 26, 2024

Great.

split the fixes into 2

You can soft reset 2 commits back so that all of the Fixup changes are there together. Actually, mixed reset might be better in this case. Mixed will unstage the committed changes while soft reset will keep them staged. git reset --mixed HEAD^2 I think.

Then stage one of the changes like the app/core/admin.py change that could become the fixup for the ...register admin... commit.
So have just that one change staged, then run git commit --fixup :/"register admin" and it will create a fixup commit for the last commit in history with a short msg that matches that partial string. git log-hist will show you that the fixup commit starts with "!fixup", with the full short-msg of the commit it's fixing.

autosquash commit

You can try doing the fixup now by running an interactive rebase git commit -i --autosquash HEAD^10. There's only 8-9 commits from HEAD back to 'hackforla/peopledepot/main', so 10 will go back enough to cover all of your changes. It'll open an editor with the rebase plan, and it'll move the fixup commit to right after the commit it's fixing up, with a fixup action in that line. Just close the editor and it will apply it correctly.

Then you can create the fixup commits for the other 2 things and apply them together. I just realized the remaining changes is actually one thing, which is fixing the ...import model... commit. I just wanted to say you can apply multiple fixups in the same autosquash commit.

After that, you should've combined all the fixes into the original commits and still have the exact same code as when the PR was approved. You can check using git diff origin/main I think. Then you can force push to replace what's in the PR.

why I'm doing this

In open source projects, some (many?) maintainers like to keep the main branch history clean and linear. So no fix commit for something earlier in the same PR and no merge commits. So they want you to rebase your commits to the latest main if main was changed before your PR.

They might do the cleanup themselves, ask contributors to do it, or have a merge team that's very adept at handling this.

Thanks for wanting to try this. I wanted to give people this experience that they could encounter in the wild, but I also understand some people are at the stage where they're already learning several things at the same time and don't want another complication.

@fyliu
Copy link
Member

fyliu commented Aug 26, 2024

I can't remember the way to do it with a tool (lazygit) without using the computer, so I'll tell you later. That tool might do several git commands in the background for every action you tell it to take. You can do things like browse an old commit and tell it to commit part of it as a separate commit, splitting it into 2 at that point in history, with all the later commits after them.

@fyliu
Copy link
Member

fyliu commented Aug 26, 2024

lazygit instructions

I did 2 things:

  1. moved the pre-commit commit to before the "docs(add-model)" commit and applied it as a fixup.
  2. split the last commit into 2 and moved and applied them to the correct commits in history.

move and apply fixup

  1. assume you're running lazygit and on the correct branch
  2. focus on the commit panel by pressing "4"
  3. move focus to the "pre-commit" line by pressing "j" and "k" to move the cursor down and up
  4. move the commit down a position by pressing "ctrl-j"
  5. apply the fixup by pressing "f" and "enter"

split a commit into 2

  1. assume you're running lazygit and on the correct branch
  2. focus on the commit panel by pressing "4"
  3. move focus to the commit you want to split by pressing "j" and "k" to move down and up. in this case, the top commit is where we want to be
  4. explore the commit by pressing "enter"
  5. stage the admin.py file by focusing on it and pressing "space"
  6. call up the patch menu by pressing "ctrl-p"
  7. select "Move patch into new commit" or press "n"
  8. enter a new temporary commit message like "fix admin class name" and press "enter"
  9. the operation may take a number of seconds.

move and apply fixup

same steps as above with the 2 newly split commits

@dmartin4820
Copy link
Member Author

I'll give it a try. Definitely not opposed to learning more about Git

@dmartin4820
Copy link
Member Author

So I did the fixups using lazygit. It was easier that way. It got rid of the "pre-commit" commit and fixedup the commits with the changes you suggest to the admin class name and docs.

@fyliu fyliu merged commit c410944 into hackforla:main Aug 26, 2024
1 check passed
@fyliu
Copy link
Member

fyliu commented Aug 26, 2024

@dmartin4820 looks great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅Done
Development

Successfully merging this pull request may close these issues.

Create Table: check_type 😄
2 participants