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 #12228: Changes in the overview section applied #12256

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

nitishfy
Copy link
Contributor

@nitishfy nitishfy commented Oct 5, 2022

Fix: #12228

@rnveach
Copy link
Member

rnveach commented Oct 5, 2022

Please look at CI failure and fix:
https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=11497&view=logs&j=69ff11bc-5a77-53a4-545f-8dfc2c866a0e&t=fe4ed25a-cc1d-5fd5-85b9-28f4310f8666

The PR is based on a master that is 26 commit(s) old.
The max allowed is 10.
This PR is too old and should be rebased on origin/master.

@romani
Copy link
Member

romani commented Oct 5, 2022

This page explains how to pull latest and rebase https://checkstyle.org/beginning_development.html#Starting_Development

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 5, 2022

@romani @rnveach My current branch is already up to date

@rnveach
Copy link
Member

rnveach commented Oct 5, 2022

@NitishKumar06 CI and Github says it is not. Maybe confirm you have forced pushed your branch and all its updates to the server.

https://github.com/NitishKumar06/checkstyle/tree/Nitish/12228

This branch is 1 commit ahead, 27 commits behind checkstyle:master.

@romani
Copy link
Member

romani commented Oct 5, 2022

Do single commit in PR.
Follow https://checkstyle.org/beginning_development.html#Starting_Development

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 6, 2022

@romani The checks are passing now! Please review and suggest some changes if necessary.

@rnveach
Copy link
Member

rnveach commented Oct 6, 2022

@NitishKumar06 There are 2 commits in this PR. We are looking for there to only be 1, which is only your work. No merge commits.
https://github.com/checkstyle/checkstyle/pull/12256/commits

NitishKumar06 wants to merge 2 commits into checkstyle:master from NitishKumar06:Nitish/12228

See https://checkstyle.org/beginning_development.html#Starting_Development step 5 about squashing, rebasing, and force pushing.

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 7, 2022

@rnveach Gitbash is showing only 1 commit when I type the command git log.
commit

@rnveach
Copy link
Member

rnveach commented Oct 7, 2022

I don't know the Git command line to assist. I just know that Github shows there are 2 commits and the 2nd one is a merge commit.

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 7, 2022 via email

@nrmancuso
Copy link
Member

Try to hard reset back to 222bb9f and force push

@rnveach
Copy link
Member

rnveach commented Oct 7, 2022

Your display may be cut off, but git log on my local is displaying the merge commit.

> git log
commit b5367f5eb65f3cc85c275c15a481327dfcf2c90e (HEAD, z_NitishKumar06/Nitish/12228)
Merge: 222bb9f57 70bbcd835
Author: Nitish Kumar <justnitish06@gmail.com>
Date:   Wed Oct 5 22:07:13 2022 +0530

    Merge branch 'checkstyle:master' into Nitish/12228

commit 70bbcd835e7d51062abd2801d3329b791854891c (z_Kevin222004/master)
Author: rnveach <rveach02@gmail.com>
Date:   Sun Oct 2 08:20:04 2022 -0400

    Issue #12241: removes package private constructors and pmd association

commit 222bb9f57c11c1d64e736ccb2e77682a0ff483a4
Author: NitishKumar06 <justnitish06@gmail.com>
Date:   Wed Oct 5 19:15:56 2022 +0530

    Issue #12228: Changes in the overview section applied

commit a78224b04c5da7b8c52a2e2eaa018d98b0c811fd
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Oct 4 21:18:41 2022 +0000

    dependency: bump pitest-maven from 1.9.6 to 1.9.7

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 7, 2022

@rnveach it's not showing for me. Any suggestions on how to proceed further?

@rnveach
Copy link
Member

rnveach commented Oct 7, 2022

Hard reset to the non-merge commit mentioned in this PR and do a rebase on the latest master. Do not do a merge.

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 8, 2022

@rnveach This sounds a little complex! Should i just close this PR and raise a new PR for the same?

@romani
Copy link
Member

romani commented Oct 8, 2022

Use same PR, just push with force to existing remote branch.

@rnveach
Copy link
Member

rnveach commented Oct 8, 2022

I recommend trying to learn so it will help in the future. It should be simple commands, but it depends on your familiarity with git.

git checkout 12228
git reset --hard 222bb9f57c11c1d64e736ccb2e77682a0ff483a4
git rebase origin/master

Note: I do not use Git command line often, and the checkout and origin remote will depend on your local setup.

@nitishfy
Copy link
Contributor Author

@romani and @rnveach This PR is ready to be merged now!

@rnveach
Copy link
Member

rnveach commented Oct 11, 2022

@NitishKumar06 It is still not squashed. You have 3 commits now because you did another merge.

@nitishfy
Copy link
Contributor Author

@rnveach git log doesn't show me any type of merge commit which needs to be squashed. Infact I believe, when the checks are passing well, why is there a need to squash the commits?

@rnveach
Copy link
Member

rnveach commented Oct 11, 2022

We are working to add a CI item for this, #12259 , and as we found there is some issue with your local that it isn't showing those commits.

I don't remember all the specifics, but merge commits sometimes messes up the display of GH, and we as a group don't do merge commits. Everything should be rebased on top of master. When we merge PRs, we don't do a merge but another rebase. We prefer a single git history flow.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

src/xdocs/config.xml Outdated Show resolved Hide resolved
@rnveach
Copy link
Member

rnveach commented Oct 11, 2022

@NitishKumar06 Looks like you got the commits to squash into 1 and rebased.

@nitishfy
Copy link
Contributor Author

nitishfy commented Oct 11, 2022 via email

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge

@romani
Copy link
Member

romani commented Oct 11, 2022

GitHub, generate website

@romani romani assigned rnveach and unassigned romani Oct 11, 2022
@github-actions
Copy link
Contributor

src/xdocs/config.xml Outdated Show resolved Hide resolved
src/xdocs/config.xml Outdated Show resolved Hide resolved
src/xdocs/config.xml Show resolved Hide resolved
@rnveach
Copy link
Member

rnveach commented Oct 27, 2022

GitHub, generate website

@github-actions
Copy link
Contributor

@nrmancuso
Copy link
Member

@NitishKumar06 please make CI happy, you need to rebase on latest master and fix spelling errors

@rnveach
Copy link
Member

rnveach commented Oct 28, 2022

2 of my comments are not done also.

@nitishfy
Copy link
Contributor Author

@rnveach The changes are applied now!

@nrmancuso
Copy link
Member

@rnveach The changes are applied now!

@NitishKumar06 please reply “done” on each item to make it clear

@nitishfy
Copy link
Contributor Author

@nrmancuso This PR is ready to be merged now!

@nitishfy nitishfy requested a review from rnveach October 28, 2022 17:23
@rnveach rnveach merged commit 3bbdf40 into checkstyle:master Oct 28, 2022
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.

Overview Section is quite confusing
4 participants