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

DOC: Clarify TETE description #11169

Merged
merged 1 commit into from Dec 17, 2020

Conversation

NikitaTewary
Copy link
Contributor

@NikitaTewary NikitaTewary commented Dec 16, 2020

Fixes #11109

Description

Two small changes done in order to give some clarity in the TETE Documentation.
The following image shows the changes that i made.

EDIT: Formatting

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks @NikitaTewary for contributing to Astropy! 🎉

This looks good to me, but pinging @mkbrewer, @mhvk and @StuartLittlefair for a second pair of eyes just in case. I don't think a changelog entry is needed.

@pllim pllim changed the title Mycontributiongiven DOC: Clarify TETE description Dec 16, 2020
@astrojuanlu
Copy link
Member

@NikitaTewary One comment: I see your commit messages are Mycontributiongiven - please add a sentence in English describing the changes made in the commit, see https://docs.astropy.org/en/stable/development/workflow/git_edit_workflow_examples.html#commit-your-change for an example.

@pllim
Copy link
Member

pllim commented Dec 16, 2020

Hello and thank you for your contribution! I cancelled Actions CI just now for this PR, please do not be alarmed.

p.s. Would also be nice that the commits be squashed into one commit before merge, once all the comments have been addressed.

@NikitaTewary
Copy link
Contributor Author

Okay

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

There is a merge commit in the history now, please squash all the commits into one. Perhaps you might find https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#how-to-squash helpful.

Also, you can add "[ci skip]" in your final commit message to skip CircleCI, as it is unnecessary.

Thank you!

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Hello @NikitaTewary, I see that you force pushed your branch in an attempt to rebase your changes and linearize your history (good!). However, your branch ended up containing commits from other people. This is normal at the beginning, git is hard and we've all been there :)

Notice that your branch now departs from an older version of master in Astropy, as you can see here:

https://github.com/astropy/astropy/network

To solve this, you will have to do a rebase. Steps:

  1. Double check your setup https://docs.astropy.org/en/stable/development/workflow/development_workflow.html#double-check-your-setup
  2. If necessary, add a second remote https://docs.astropy.org/en/stable/development/workflow/get_devel_version.html#tell-git-where-to-look-for-changes-in-the-development-version-of-astropy
  3. Rebase https://docs.astropy.org/en/stable/development/workflow/development_workflow.html#how-to-rebase ℹ️ After this step, you should only have two commits ℹ️
  4. Force push https://docs.astropy.org/en/stable/development/workflow/development_workflow.html#how-to-push
  5. And for perfection, squash your commits into one https://docs.astropy.org/en/stable/development/workflow/development_workflow.html#how-to-squash and force push again

Let us know if you need help.

@pllim
Copy link
Member

pllim commented Dec 17, 2020

There is still one unrelated commit.

@astrojuanlu
Copy link
Member

The commit message leads one to think that f282693 does not belong here, but in fact it does. Not sure why it ended up like that though.

@NikitaTewary you followed the instructions I gave perfectly, thanks! But @pllim has a point, these two commits should indeed be one. For this, the best is to do a "squash on interactive rebase":

https://docs.astropy.org/en/stable/development/workflow/development_workflow.html#how-to-squash

which will present you this output:

pick f282693cc TST: Older pytest missing PytestUnhandledThreadExceptionWarning
pick a58c075be TIRS added for more clarity in the documentaion

and so you have to change it by this:

pick f282693cc TST: Older pytest missing PytestUnhandledThreadExceptionWarning
squash a58c075be TIRS added for more clarity in the documentaion

And then pick a final commit message (no need to write Mycontributiongiven at the end, and beware of typos though).

After this change, this is ready to merge.

@pllim
Copy link
Member

pllim commented Dec 17, 2020

@astrojuanlu , TST: Older pytest missing PytestUnhandledThreadExceptionWarning is actually from #11167 . It should not be here at all. FYI.

@astrojuanlu
Copy link
Member

Yes, the message TST: Older pytest missing PytestUnhandledThreadExceptionWarning corresponds to 32e1543, but even if f282693 contains the same message (not sure why) the diff is actually corresponding to this change, that's why I say @NikitaTewary needs to squash them.

@mkbrewer
Copy link
Contributor

Sorry that my request for a minor change caused so much trouble here.

@astrojuanlu
Copy link
Member

No fuss @mkbrewer ! These small requests are the ones that are more palatable for newcomers. The back-and-forth with git is absolutely normal, I apologize if we made too much noise (if there is a specific channel in the Slack/Matrix chat for these kind of exchanges and it's preferred that they're moved there, please let me know)

@astrojuanlu
Copy link
Member

astrojuanlu commented Dec 17, 2020

Last thing @NikitaTewary, I promise: the first line of the commit message is still misleading, please do

$ git commit --amend -m "TIRS added for more clarity in the documentation"

(Edit: typo)

and force push again.

@NikitaTewary
Copy link
Contributor Author

@astrojuanlu @mkbrewer @pllim thank you for being so patient. I am a newcomer and I learnt a lot of new commands in git because of your directions, I just have one request if you could assign me more docs related issues I would love to fix it.
Thank you!

Copy link
Member

@astrojuanlu astrojuanlu 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 good to me now!

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

The PR is in good shape now. However the commit author in the history is a little misleading because of the squash and what-nots. If you don't mind that being messed up, feel free to merge. Otherwise, it needs a git commit --amend to change the authorship. 🤷

@NikitaTewary
Copy link
Contributor Author

@pllim what should be the desired Author name? My name or something else?

@pllim
Copy link
Member

pllim commented Dec 17, 2020

It currently shows me and you both as author, but in reality you are the author.

@saimn
Copy link
Contributor

saimn commented Dec 17, 2020

git commit --amend --author="Author Name <email@address.com>" --no-edit

where you put your name and email to get the proper credit.

@NikitaTewary
Copy link
Contributor Author

C:\Users\Asus\OneDrive\Desktop\Astropy\astropy>git push -forced
fatal: the receiving end does not support push options
fatal: the remote end hung up unexpectedly
error: failed to push some refs to 'https://github.com/NikitaTewary/astropy.git'

Now, this is what is popping up whenever I am giving the push command, after I tried to change the Author

@astrojuanlu
Copy link
Member

Weird! It was working for you before, right? I think it's a typo in --forced, (it should rather be --force, without d)

@astrojuanlu astrojuanlu merged commit 7c01ae1 into astropy:master Dec 17, 2020
@astrojuanlu
Copy link
Member

This is perfect now. Thanks everyone for the patience and congratulations @NikitaTewary for your first contribution 🎉

eteq pushed a commit that referenced this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minor clarifications needed in TETE documentation.
5 participants