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.

Sign up for GitHub

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

Jump to bottom

update python dependencies & build sections #259

Merged
merged 4 commits into from
Apr 22, 2022
Merged

update python dependencies & build sections #259

merged 4 commits into from
Apr 22, 2022

Conversation

egpbos
Copy link
Member

@egpbos egpbos commented Apr 6, 2022 •
edited
Loading

This is a continuation and further update to 2022 of #218. To reiterate from #218:

Primarily, I added the recommendation from the discussion in #156. Simultaneously, I merged the two sections on Python dependencies & packages (I think the Building packages should be a subsection here, it's really intertwined). Also trimmed the existing text here and there, it was rather verbose, with lots of examples of commands that you can also easily find online with the right keywords (which we provide here).

On top of that, this PR has a few extra tweaks:

  • I added a note about using Anaconda servers, as we discussed in the Software Sustainability SIG and elsewhere.
  • Made venv the first choice instead of virtualenv.
  • Removed all mention of setup.py in favor of setup.cfg and pyproject.toml, since setup.py has been deprecated in favor of those configuration files.
  • Removed TravisCI mentions.

This was referenced Apr 6, 2022
Closed
Merged
Closed
@egpbos egpbos requested a review from bouweandela April 6, 2022 14:03
@egpbos
Copy link
Member Author

egpbos commented Apr 7, 2022

Note to reviewer: the link checker is failing because of dead links in master. We should make a separate PR to fix them.

@LourensVeen LourensVeen mentioned this pull request Apr 7, 2022
Merged
@egpbos
Copy link
Member Author

egpbos commented Apr 8, 2022

Any further comments on this? Note that everything in 3bf470e was already reviewed by @bouweandela in 2020. I changed a few things in the three commits after that, so for ease of reviewing, just checking those three commits should suffice.

bouweandela
bouweandela reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks for the much needed updates!

best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Show resolved Hide resolved
best_practices/language_guides/python.md Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
best_practices/language_guides/python.md Show resolved Hide resolved
best_practices/language_guides/python.md Outdated Show resolved Hide resolved
egpbos added a commit that referenced this pull request Apr 8, 2022
@egpbos @bouweandela
apply review comments
d6ee7e8 
The first comments were given in #219 already, but still apply here.
Later also in #259 comments were
added that were applied in this commit. Furthermore, some additional
minor edits and an update of the wheels description.

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
@egpbos
Copy link
Member Author

egpbos commented Apr 8, 2022

Rebased loose review commits into one big one for cleaner history.

@egpbos egpbos requested a review from bouweandela April 8, 2022 10:15
@egpbos
Copy link
Member Author

egpbos commented Apr 13, 2022

@bouweandela anything further before approving? :)

@bouweandela
Copy link
Member

Could you please have a look at the unresolved comments? And at the merge conflict?

@egpbos
Copy link
Member Author

egpbos commented Apr 14, 2022 •
edited
Loading

I replied to the unresolved comments, waiting for your reply :) I will resolve the merge conflict, it's just because I was doing 5 separate PRs in a row on the same file.

egpbos added a commit that referenced this pull request Apr 14, 2022
@egpbos @bouweandela
apply review comments
b9570bc 
The first comments were given in #219 already, but still apply here.
Later also in #259 comments were
added that were applied in this commit. Furthermore, some additional
minor edits and an update of the wheels description.

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
@egpbos
Copy link
Member Author

egpbos commented Apr 14, 2022

Rebased to fix merge conflicts. Can you take a look at the two unresolved discussions?

@bouweandela
Copy link
Member

bouweandela commented Apr 15, 2022 •
edited
Loading

My apologies, I wrote replies to your comments a few days ago but it looks like I did not use the 'Finish your review' button, so you couldn't see them. And then I was waiting for you to answer them, which obviously didn't work 😆

@egpbos @bouweandela
apply review comments
74c119c 
The first comments were given in #219 already, but still apply here.
Later also in #259 comments were
added that were applied in this commit. Furthermore, some additional
minor edits and an update of the wheels description.

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
@egpbos
Copy link
Member Author

egpbos commented Apr 20, 2022

@bouweandela comments resolved, please make a final check :)

@egpbos egpbos requested a review from bouweandela April 20, 2022 11:26
bouweandela
bouweandela approved these changes Apr 22, 2022
View reviewed changes
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks!

@bouweandela bouweandela merged commit 18eedee into master Apr 22, 2022
@bouweandela bouweandela deleted the python_deps_and_build branch April 22, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bouweandela bouweandela bouweandela approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@egpbos @bouweandela