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

Issue #6276 fix #6296

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Conversation

Shurazi
Copy link

@Shurazi Shurazi commented Sep 2, 2024

Preserve HTTP Parameter order by using a LinkedHashMap instead of HashMap in HttpUtil:getTableMap

.vscode/settings.json Outdated Show resolved Hide resolved
client/.project Outdated Show resolved Hide resolved
@jonbartels
Copy link
Contributor

Linking to parent issue #6276

@rogin
Copy link

rogin commented Sep 10, 2024

Created a PR to include a unit test.

@tonygermano
Copy link
Collaborator

tonygermano commented Sep 11, 2024

You only want the changes to HttpUtil in this PR, and not all of the other stuff. There are several ways to correct this. Here are a couple.

If you don't actually care about keeping any of the other changes, do an interactive rebase with your bug6276 branch checked out:

  1. git rebase -i HEAD~3 include the previous 3 commits
    1. drop the 1st and 3rd commit
    2. pick the 2nd
    3. save the file and exit
  2. git push -f force push to override your github branch, which now contains only a single commit changing a single file

If you want to keep your changes to the .project and .gitignore files locally, again do this with your bug6276 branch checked out. This is actually closer to what your normal workflow should be (selectively staging files rather than committing everything) when you don't want to include in the PR all of the files that inadvertently change because of stuff the IDE is doing:

  1. git reset development resets the index so that your bug branch points to the same commit as the development branch, and all of your changed files remain (unstaged) in the working tree.
  2. git add core-util/src/com/mirth/connect/util/HttpUtil.java stage only this file
  3. git commit -m "Use LinkedHashMap to preserve parameter order" commit only the single staged file (do NOT use the -a/--all option)
  4. git push -f force push to override your github branch, which now contains only a single commit changing a single file
  5. Your changes to the other files are still around if you want to do something with them.

The force push at the end of either method will automatically update this PR to remove the unwanted commits.

@Shurazi
Copy link
Author

Shurazi commented Sep 12, 2024

As suggested by Tony, I used his rebase method to omit the unwanted changes from my previous commits.
I appreciate the feedback. This is my first PR. Hoping the fork branch is correct now.

@rogin
Copy link

rogin commented Sep 12, 2024

Seems you rebased off the wrong branch as it contains changes for a separate issue.

@Shurazi
Copy link
Author

Shurazi commented Sep 12, 2024

Rebased and pushed again to remove unrelated changes.

@rogin
Copy link

rogin commented Sep 12, 2024

Nice. Could you also merge by PR listed above which includes a unit test? Once you merge it, it should show up here without any extra steps.

@Shurazi
Copy link
Author

Shurazi commented Sep 12, 2024

@rogin - I merged your PR, but it looks like yours had some unrelated changes as well.
Thinking I need to undo that merge and let you rebase/clean up your branch to take out the extra changes?

@tonygermano
Copy link
Collaborator

tonygermano commented Sep 13, 2024

@Shurazi starting with your bug branch checked out

  1. git reset --hard 4b148e4a1 hard reset the branch back to your commit prior to merging the PR
  2. git cherry-pick 30d27609b pick just the commit from the merge that contains the changes to HttpUtilTest.java
  3. git push -f force push the changes

The reason the original commits came back in the merge was because @rogin created his branch before you cleaned up your side. A rebase in @rogin's fork prior to merging would have cleaned that up.

(cherry picked from commit 30d2760)
@jonbartels
Copy link
Contributor

@Shurazi - Excellent work! I am very happy that you and tonygermano and rogin worked together to clean up the PR. Sometimes the cleaning is more work than the original code change!

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.

4 participants