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

small cleanup #137

Merged
merged 3 commits into from
Nov 23, 2021
Merged

small cleanup #137

merged 3 commits into from
Nov 23, 2021

Conversation

MCWertGaming
Copy link
Collaborator

This PR some smaller things left over from #132.

cpp-terminal/base.cpp Outdated Show resolved Hide resolved
cpp-terminal/base.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is fine.

Before you merge, can you please create a better commit history? You can either squash it to one commit, or have more commits as needed, and have a nice git commit log.

For example, you can have one commit which says "use single quotes instead of double quotes", and another commit which says "Remove save_screen(), already handled by the constructor". You can write more, but at least such one line should be part of every commit. Commits like "small cleanup" are fine for development, but one should fix that before merging.

If you learn to do this, it will become second nature. It makes your code and PRs much easier for others to review and see, and it makes the git history much easier to navigate, bisect and learn what was done and why.

@MCWertGaming
Copy link
Collaborator Author

Yeah sure, I'll make it like that next time.

@MCWertGaming MCWertGaming merged commit 3ef3ff4 into jupyter-xeus:master Nov 23, 2021
@MCWertGaming MCWertGaming deleted the cleanUp branch November 23, 2021 16:48
@certik
Copy link
Collaborator

certik commented Nov 23, 2021

@MCWertGaming I asked if you could please do it this time. Since you already merged, let's leave it like that. But please let's not merge anymore commits like "small cleanup".

@MCWertGaming
Copy link
Collaborator Author

@MCWertGaming I asked if you could please do it this time. Since you already merged, let's leave it like that. But please let's not merge anymore commits like "small cleanup".

I have squashed the comits like you said. The problem with changing commits afterward is that I'm not sure how you do it without making a new commit history by yourself and pushing it without making a force push (which creates a big warning in github). @certik

@certik
Copy link
Collaborator

certik commented Nov 23, 2021

Ah I missed that. You are right. The squash commit is ok. Sorry about that.

Yes, when I do it manually, I rewrite the commits locally and force push it into the branch. I mostly use gitlab, where it doesn't generate a warning. What warning does it generate on github? The alternative is to open a new PR with the polished commits (but that seems like an overkill for a case like this).

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.

None yet

2 participants