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

Removing __all__ in modules #36

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Removing __all__ in modules #36

merged 1 commit into from
Oct 22, 2021

Conversation

effulgentstar
Copy link
Contributor

#33 Remove use of all from all modules

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

This looks great!
However, please don't remove the .gitignore file (mentioned in the inline comment as well).
Otherwise this would be ready to merge ☺️

After this one is done, could you make a similar change in kcidb?

.gitignore Outdated
@@ -1,3 +0,0 @@
__pycache__
*.egg-info
*.pyc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not remove this file, we need it 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you :) However, this file still shows the change because the newline was removed from the last line. Could you fix that? You can just fetch the previous version of the file with something like git checkout HEAD^ -- .gitignore, and then amend the file in the last commit with git commit --amend --no-edit .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did what you asked me to. Is it okay now?

@spbnick
Copy link
Collaborator

spbnick commented Oct 22, 2021

The change looks good now, but there's a merge commit we don't need. Could you please remove it?
Write if you need help with that :)

@effulgentstar
Copy link
Contributor Author

Yes, I realized that and I do need help removing it

@spbnick
Copy link
Collaborator

spbnick commented Oct 22, 2021

Yes, I realized that and I do need help removing it

No problem! First, let's see how your remotes are setup. Could you post the output of git remote -v command in your repo?

@effulgentstar
Copy link
Contributor Author

$ git remote -v
origin https://github.com/effulgentstar/kcidb-io.git (fetch)
origin https://github.com/effulgentstar/kcidb-io.git (push)

@spbnick
Copy link
Collaborator

spbnick commented Oct 22, 2021

$ git remote -v
origin https://github.com/effulgentstar/kcidb-io.git (fetch)
origin https://github.com/effulgentstar/kcidb-io.git (push)

Alright, first let's add the remote for this repo (the one you're contributing to, as opposed to your fork that your command listed). Let's name it "upstream":

git remote add upstream git@github.com:kernelci/kcidb-io.git

You can check that this worked by running git remote -v - you should see four lines of output, two for your old "origin" remote, and two for the new "upstream" remote.

Then fetch from the remote (or "repo") that you just added:

git fetch upstream

Now you're ready to "rebase" your changes on top of what's in the "main" branch of this ("upstream") repo:

git rebase upstream/main

The above should complete without a problem.

Then you should be able to preview the difference between this repo's main branch, and your local branch, like this:

git log --oneline upstream/main..

It should output something like this:

e56a266 (HEAD -> lucy) Removing __all__ in modules

If this is what you see (or close to, but still only one line of output), then you've done it, and are ready to force-push to your PR branch again!

Please ask questions if anything is unclear, good luck 😀

@effulgentstar
Copy link
Contributor Author

I was able to follow your instructions. I hope it's okay now ☺

@spbnick
Copy link
Collaborator

spbnick commented Oct 22, 2021

Yes, it's perfect now, merging! Thanks and congratulations on your first PR 🥳

@spbnick spbnick merged commit 541d7e7 into kernelci:main Oct 22, 2021
@effulgentstar
Copy link
Contributor Author

Thank you for your guidance @spbnick 👏 👏

@effulgentstar effulgentstar deleted the lucy branch October 22, 2021 18:30
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