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

regression from 5c4833e #140

Open
leggewie opened this issue Oct 3, 2023 · 5 comments · May be fixed by #141
Open

regression from 5c4833e #140

leggewie opened this issue Oct 3, 2023 · 5 comments · May be fixed by #141

Comments

@leggewie
Copy link
Contributor

leggewie commented Oct 3, 2023

please revert commit 5c4833e which introduces a couple of regressions. in the future it would be nice if proper code inspection "git log -p" were to prevent commits that do not merge rebased code but simply overwrite other contributions.

ping @jeslynlamxy ping @Kalebu

@olichuuwon
Copy link
Contributor

in addition to my response to this comment:
5c4833e#r128977068

i was unaware of the regressions as i could have been using an older version that worked instead, did not intent to "overwrite" your contributions, like i mentioned just trying to propose an additional function :)

@leggewie
Copy link
Contributor Author

leggewie commented Oct 3, 2023

sure, but you should really inspect the code you are pushing. "git log -p" is your friend. please also make sure to learn how to use git branches and git merge and git rebase correctly. proper use of the git workflow (and proper inspection of code changes by the maintainer) would have prevented this.

@jeslynlamxy please go ahead and push a change that rebases your code on top of existing code instead of overwriting it. It is your responsibility to clean up after yourself.

And no, you weren't using an older version and thus unaware of changes upstream. 5c4833e is your commit, signed by you. It is sad you do not know how to inspect git history so you have to guess (incorrectly) in this regard.

@leggewie
Copy link
Contributor Author

leggewie commented Oct 3, 2023

#127 discusses the issue you were trying to address. The issue is fixed but the ticket is still open. github can automatically close a ticket when a changeset is merged IF the proper commit message is used.

regressing on the following, previously fixed issues
#104
#105
#108
reverted 3611a0c
possibly others

@olichuuwon
Copy link
Contributor

olichuuwon commented Oct 3, 2023

I express my appreciation for your feedback on the recent code changes. I understand your concerns regarding the regressions introduced by the recent commits.

I'd like to clarify that my intention was to propose an additional function to address an issue I encountered. I certainly didn't intend to introduce poor code or regressions intentionally, and if my changes have caused any issues, I genuinely apologize for any inconvenience they may have caused.

While I'm responsible for my contributions, the code manager or maintainer typically plays a significant role in reviewing, approving, and merging changes into the codebase.

Additionally, I kindly request that we maintain a respectful and constructive tone in our interactions. Harassment or hostile behaviour seen can be detrimental to the collaborative spirit of any project.

Please understand that I may not be able to respond promptly to further messages (away). I appreciate your understanding and patience.

Thanks lots. :)

@leggewie
Copy link
Contributor Author

leggewie commented Oct 4, 2023

@olichuuwon Noted. Please direct your effort now to fixing the issue you created.

@leggewie leggewie linked a pull request Oct 4, 2023 that will close this issue
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 a pull request may close this issue.

2 participants