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

Fixed typos in docstring of subroutines.py #126

Closed
wants to merge 5 commits into from
Closed

Conversation

Fil158
Copy link

@Fil158 Fil158 commented Oct 29, 2024

I fixed some grammar and general typos

@bobleesj
Copy link
Contributor

@Fil158 for training purposes, could you please add a news file as instructed? Please refer to other PRs

@bobleesj
Copy link
Contributor

@Fil158 As discussed via slack, let's also make this PR title a bit more descriptive

@Fil158
Copy link
Author

Fil158 commented Oct 30, 2024

I added the doc.rst on my local doc branch
in the news rep, with the comment under Fixed
Now, reading the instructions you provided me, that are clear till this point, it is unclear how to proceed...

@Fil158 Fil158 changed the title Fixed some typos Fixed typos in docstring of subroutines.py Oct 30, 2024
@bobleesj
Copy link
Contributor

I added the doc.rst on my local doc branch in the news rep, with the comment under Fixed Now, reading the instructions you provided me, that are clear till this point, it is unclear how to proceed...

Could you please share which part of the instructions is not clear to you? You can also refer to other PRs on how they create news file.

@Fil158
Copy link
Author

Fil158 commented Oct 30, 2024

image

@bobleesj
Copy link
Contributor

That's not a formal part of the numbered step-by-step instructions. It's under Now, whenever you work in the repo Please explain which is not clear because I can also read that you've pasted.

@bobleesj
Copy link
Contributor

Simply - you can push your news files by using git commit -m "\<summary of what the changes were\>" and git push

@bobleesj
Copy link
Contributor

@Fil158

For testing your package via Pytest, please refer to How to test your package with specified Python version on GitLab. For your training, please also share the output of the Terminal test result here.

@Fil158
Copy link
Author

Fil158 commented Oct 30, 2024

Ok that is what I was asking, just add, commit and then push the doc.rst
Right?

@bobleesj
Copy link
Contributor

Ok that is what I was asking, just add, commit and then push the doc.rst Right?

Alright, next time, you could save time by providing the course of action that you are about to take and I can confirm by giving you saying "yes" or thumbs-up.

@Fil158
Copy link
Author

Fil158 commented Oct 30, 2024

Now I'm starting to test: I'm in ..\diffpy.snmf
Is it the right command to paste?
conda create -n diffpy_snmf_env python=3.13

@bobleesj
Copy link
Contributor

bobleesj commented Oct 30, 2024

@Fil158 Instead of Removed undesirable comment, Remove extra <news items> under Fixed in news file is better for me (reviewer) so confirm that you've modified per the feedback.

@bobleesj
Copy link
Contributor

In this case, the instruction appears to be clear: First, cd into the package directory, then follow these instructions: Please try first?

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

please see my comment. Thanks for the hard work Filippo and Bob!


**Fixed:**

Fixed 3 typos in docstring of subroutines.py
Copy link
Contributor

Choose a reason for hiding this comment

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

please see comment in the other PR. We don't need a news item for just correcting typos.

@bobleesj
Copy link
Contributor

@Fil158 Did the local pytest run for you? Please also address the in-line comment regarding news.

@bobleesj
Copy link
Contributor

bobleesj commented Nov 1, 2024

@Fil158 For us to be efficient, it would be appreciated if you could reply on GitHub comments within a reasonable time period of say within 24 hrs during normal working hours.

@Fil158 Fil158 closed this Nov 6, 2024
@bobleesj
Copy link
Contributor

bobleesj commented Nov 6, 2024

@Fil158 appears you've closed this issue without replies. Will another PR be created by you? Before closing any issues/PRs, since they are all used for bookkeeping and documenting purposes, please address the context.

@sbillinge sbillinge reopened this Nov 6, 2024
@sbillinge
Copy link
Contributor

reopening. We don't want to lose the conversation without resolving it.

@bobleesj
Copy link
Contributor

bobleesj commented Nov 7, 2024

@Fil158 Do you not receive notifications (emails)? I am just afraid whether things aren't configured properly since Billinge and I created multiples questions and follow-ups that would require your reply.

@Fil158
Copy link
Author

Fil158 commented Nov 7, 2024

Oh sorry, I wrote you on slack @bobleesj that local pytest run correctly

@Fil158
Copy link
Author

Fil158 commented Nov 7, 2024

No new PR on this topic

@bobleesj
Copy link
Contributor

bobleesj commented Nov 7, 2024

No new PR on this topic

because?

@Fil158
Copy link
Author

Fil158 commented Nov 7, 2024

@bobleesj please see the new PR #135

@bobleesj
Copy link
Contributor

bobleesj commented Nov 7, 2024

@Fil158 you can close this PR then - but it might be useful to a single sentence here why we've decided to make a new PR and reference the other PR link i.g. "Closing this PR - a new PR with a clean commit history is created here: #135"

@sbillinge
Copy link
Contributor

@Fil158 you can close this PR then - but it might be useful to a single sentence here why we've decided to make a new PR and reference the other PR link i.g. "Closing this PR - a new PR with a clean commit history is created here: #135"

Thanks @bobleesj . @Fil158 Bob is right. When you write the comments don't treat this like a chat thread on a phone where it is only supposed to communicate a conversation between two people who know what is going on, but treat it as if you are leaving enough information that some new person in the future reading the thread can reconstruct what is going on.

@sbillinge
Copy link
Contributor

Closing. Replaced by #135

@sbillinge sbillinge closed this Nov 7, 2024
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.

3 participants