Addressing Issue 85 #87
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbillinge
reviewed
Mar 19, 2024
README.rst
Outdated
| @@ -116,7 +112,7 @@ version using :: | |||
| With other Python distributions the program can be upgraded to | |||
| the latest version as follows :: | |||
|
|
|||
| easy_install --upgrade diffpy.pdfmorph | |||
| pip install diffpy.pdfmorph | |||
There was a problem hiding this comment.
this should be conda update, not pip.
| @@ -77,7 +78,7 @@ conda and test that it is working by opening a terminal and typing | |||
| To create and activate a conda environment to use this software, run | |||
| the following command from the command line :: | |||
|
|
|||
| conda create -n pdfmorph_env python=3 | |||
| conda create -n pdfmorph_env python=3 --yes | |||
| source activate pdfmorph_env | |||
|
|
|||
| If you're using Windows, replace ``source activate pdfmorph`` with | |||
There was a problem hiding this comment.
please check this. I think that maybe this is no longer needed.
sbillinge
reviewed
Mar 26, 2024
README.rst
Outdated
| @@ -116,7 +113,7 @@ version using :: | |||
| With other Python distributions the program can be upgraded to | |||
| the latest version as follows :: | |||
|
|
|||
| easy_install --upgrade diffpy.pdfmorph | |||
| conda update diffpy.pdfmorph | |||
There was a problem hiding this comment.
This strikes me as not correct based on the text just above. I suggest that we simply delete lines starting "with other python distributions......" and just leave instructions how to update the software using conda.
|
Oops! I didn’t realize it would notify you when I pushed. Currently working on it!
On Mar 25, 2024, at 9:42 PM, Simon Billinge ***@***.***> wrote:
@sbillinge commented on this pull request.
nearly there. I love the badge!
please see my one new comment and there is an unresolved one from before about commands on windows.
In README.rst <#87 (comment)>:
> @@ -116,7 +113,7 @@ version using ::
With other Python distributions the program can be upgraded to
the latest version as follows ::
- easy_install --upgrade diffpy.pdfmorph
+ conda update diffpy.pdfmorph
This strikes me as not correct based on the text just above. I suggest that we simply delete lines starting "with other python distributions......" and just leave instructions how to update the software using conda.
—
Reply to this email directly, view it on GitHub <#87 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFXBRSKP6JNGYML4HVI37YDY2DHB7AVCNFSM6AAAAABE6FT23CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJZGEZTMMZUGI>.
You are receiving this because you authored the thread.
—
Caden Myers
Ph.D. Student
Materials Science & Engineering
Columbia University
***@***.***
|
… conda deactivate
|
No worries this is how it works. When you feel that you have completed
everything, add a comment something like "ready for review and merge" so we
know it is not a work on progress. It is also possible to designate it as
a draft and to keep pushing them remove the draft status when you want it
merged.
I like to see frequent pushes off unfinished work so I can give feedback if
needed earlier on the process.
S
…On Mon, Mar 25, 2024, 10:07 PM Caden Myers ***@***.***> wrote:
Oops! I didn’t realize it would notify you when I pushed. Currently
working on it!
> On Mar 25, 2024, at 9:42 PM, Simon Billinge ***@***.***> wrote:
>
>
> @sbillinge commented on this pull request.
>
> nearly there. I love the badge!
>
> please see my one new comment and there is an unresolved one from before
about commands on windows.
>
> In README.rst <
#87 (comment)>:
>
> > @@ -116,7 +113,7 @@ version using ::
> With other Python distributions the program can be upgraded to
> the latest version as follows ::
>
> - easy_install --upgrade diffpy.pdfmorph
> + conda update diffpy.pdfmorph
> This strikes me as not correct based on the text just above. I suggest
that we simply delete lines starting "with other python
distributions......" and just leave instructions how to update the software
using conda.
>
> —
> Reply to this email directly, view it on GitHub <
#87 (review)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/BFXBRSKP6JNGYML4HVI37YDY2DHB7AVCNFSM6AAAAABE6FT23CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJZGEZTMMZUGI>.
> You are receiving this because you authored the thread.
>
—
Caden Myers
Ph.D. Student
Materials Science & Engineering
Columbia University
***@***.***
—
Reply to this email directly, view it on GitHub
<#87 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOWUJNR43IST4PC77FUEDY2DJ5NAVCNFSM6AAAAABE6FT23CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJZGI2TKMZVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 75.01% 84.33% +9.32%
==========================================
Files 36 38 +2
Lines 1437 1890 +453
==========================================
+ Hits 1078 1594 +516
+ Misses 359 296 -63 ☔ View full report in Codecov by Sentry. |
sbillinge
reviewed
Apr 1, 2024
|
Sorry, I forgot to remove the draft status. I believe it is ready. I talked with Andrew about your comment on line 81 and he was thinking it's okay to leave in.
… On Apr 1, 2024, at 3:10 PM, Simon Billinge ***@***.***> wrote:
@sbillinge commented on this pull request.
LGTM. This is marked as draft. Are still working on this branch?
—
Reply to this email directly, view it on GitHub <#87 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFXBRSJ3Y6HTSULF25LRGF3Y3GWMHAVCNFSM6AAAAABE6FT23CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZRHE2DANZZGM>.
You are receiving this because you authored the thread.
|
|
Nice job, merged! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #85
Addressed:
Add --yes to conda create -n pdfmorph_env python=3 command
Remove duplicated conda config --add channels conda-forge
Change easy_install --update to pip
Replace travis badge with gh status badge
I believe bg-mpl-stylesheets is downloading properly as well.