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

Delete jscotto directory. #21106

Closed
wants to merge 3 commits into from
Closed

Conversation

joe-scotto
Copy link
Contributor

Description

I've been modifying all keyboards within the handwired/jscotto directory in addition to moving them to handwired/scottokeebs. I have opened individual PRs for each keyboard to move into /handwired/scottokeebs so /handwired/jscotto can be removed.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

This shouldn't have been done in this way. Each keyboard should have been directly moved in individual PRs, rather than duplicating and then removing this old folder.

@joe-scotto
Copy link
Contributor Author

joe-scotto commented Jun 7, 2023

@fauxpark I was told by @drashna that this would be the correct way to do it. I should also add that they're not really "duplicates". The way they were in the jscotto directory the keymaps weren't pristine as well as other changes that needed to be done. The new PRs I made sure to conform them as best I could to the contributing guidelines.

CleanShot 2023-06-07 at 03 21 48@2x

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

Sorry this wasn't better communicated. Per the PR checklist, keyboard moves and updates need to go through develop. The above PRs have basically duplicated your boards on master. This is a problem because master is merged into develop when commits are made to it, so any attempt to undo the duplication will propagate, and you'll have to start over targeting the correct branch.

@joe-scotto
Copy link
Contributor Author

@fauxpark Shouldn't that not matter though? This PR to delete the jscotto directory is targeting develop so once the PRs for the individual boards are merged, jscotto can just be deleted on develop.

So basically once the remaining 2 boards are added to the scottokeebs directory, jscotto can completely be removed. Then that change should follow over, no?

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

Read what I said again. The issue isn't with the develop branch.

@joe-scotto
Copy link
Contributor Author

@fauxpark Sorry this is confusing...

What should be done now then? Should I branch off develop for each board in jscotto and move the files there as well as updating them? So for example:

  1. Branch develop -> scotto9-move
  2. Move jscotto/scotto9 -> scottokeebs/scotto9
  3. Update code for that board in the new location

What would have to be done about the other boards then? Since now they're basically duplicated like you say... why can't the directory just be deleted?

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

The key part is this:

The above PRs have basically duplicated your boards on master.

https://github.com/qmk/qmk_firmware/tree/master/keyboards/handwired/jscotto
https://github.com/qmk/qmk_firmware/tree/master/keyboards/handwired/scottokeebs

These PRs should not have been merged to master; everything should have gone to develop. The only way forward here is to revert all of your changes so far and redo them properly:

This is a problem because master is merged into develop when commits are made to it, so any attempt to undo the duplication will propagate, and you'll have to start over targeting the correct branch.

@joe-scotto
Copy link
Contributor Author

@fauxpark So two questions then....

  1. How exactly can I go about reverting the changes if they've already been merged into master?
  2. Can the new PRs, targetting develop, have changes in them to the keymaps as well?

But with that said, I guess I'm not really understanding why in this case the jscotto directory can't just be deleted after the boards are merged into scottokeebs. I read the part about aliases in the contributing document and can easily update that file but to have to start all over with 5 boards doesn't make much sense to me.

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

I guess I'm not really understanding why in this case the jscotto directory can't just be deleted

Because this would only affect develop. The boards are still duplicated in master.

@joe-scotto
Copy link
Contributor Author

@fauxpark Yes, but wouldn't jscotto be deleted from master once develop gets merged? Then the boards would no longer be duplicated.

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

It would...in just under 3 months.

@joe-scotto
Copy link
Contributor Author

So should I just have jscotto deleted from master? I'm really confused on what exactly needs to happen here.

@fauxpark
Copy link
Member

fauxpark commented Jun 7, 2023

Everything needs to be reverted. Then you should move + alias each board in individual PRs targeted at develop. No need for a followup PR like this one deleting the old folder, because Git doesn't consider empty folders.

@github-actions github-actions bot added documentation via Adds via keymap and/or updates keyboard for via support labels Jun 7, 2023
@joe-scotto
Copy link
Contributor Author

@fauxpark Aliases added along with merge conflicts being resolved. Let me know if anything else needs attention on this one.

@github-actions github-actions bot added core documentation translation via Adds via keymap and/or updates keyboard for via support and removed documentation via Adds via keymap and/or updates keyboard for via support labels Jun 7, 2023
@github-actions github-actions bot added the python label Jun 7, 2023
@joe-scotto joe-scotto mentioned this pull request Jun 7, 2023
14 tasks
@joe-scotto
Copy link
Contributor Author

@fauxpark Moved to #21157.

@joe-scotto joe-scotto closed this Jun 7, 2023
@joe-scotto joe-scotto deleted the delete-jscotto branch September 27, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation keyboard keymap python translation via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants