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

address issue #282 #283

Merged
merged 10 commits into from
Aug 23, 2023
Merged

address issue #282 #283

merged 10 commits into from
Aug 23, 2023

Conversation

YeungOnion
Copy link
Contributor

addresses #282

ran a pixi init and added depends listed in environments.yml.

Needing feedback on

  • what versions for which packages we'll want for the pixi spec as I just did pixi add ...
  • whether making a CONTRIBUTING.md from the primary README.md is needed
  • verbiage and correctness in CONTRIBUTING.md

@YeungOnion
add as pixi project, mention in contributing readme
a9a5bac 
added pixi.toml with depends as well as build tasks, made CONTRIBUTING.md mostly added stuff regarding dev.
baszalmstra
baszalmstra reviewed Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@baszalmstra baszalmstra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good! I've got a few notes:

  • When writing markdown can you make sure that each sentence is on a newline. This makes diffs a lot more readable/managable.
  • More inline in the code :)

pixi.toml Outdated Show resolved Hide resolved
pixi.toml Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
YeungOnion and others added 4 commits August 22, 2023 10:33
@YeungOnion @baszalmstra
Update pixi.toml for other platforms
bdd1074 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update pixi.toml for fmt and clippy
d3cd96d 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update CONTRIBUTING.md
03a26e0 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update pixi.toml for approx version syntax
b125ad8 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
baszalmstra
baszalmstra reviewed Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

I added some comments. To split sentences over lines in markdown and to also use ~= for the other pixi dependencies. Otherwise looks really good. 👍

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
YeungOnion and others added 5 commits August 23, 2023 06:31
@YeungOnion
Merge branch 'mamba-org:main' into main
090599b
@YeungOnion @baszalmstra
Update CONTRIBUTING.md
7abc8ae 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update README.md
13a38bf 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update pixi.toml
beabed6 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion @baszalmstra
Update CONTRIBUTING.md
35e1e93 
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@YeungOnion
Copy link
Contributor Author

Thanks for putting those in, hadn't realized the first set of edits you suggested weren't all the instances needing change.

@YeungOnion
Copy link
Contributor Author

Also, is there a preference to squash commits so that the PR has fewer commits where reasonable or is it typically kept as part of the history?

@baszalmstra
Copy link
Collaborator

I usually squash and merge when I merge the PR.

@baszalmstra baszalmstra merged commit fc88e12 into conda:main Aug 23, 2023
4 of 7 checks passed
@baszalmstra
Copy link
Collaborator

Thanks @YeungOnion! Fantastic first contribution!

@YeungOnion YeungOnion mentioned this pull request Aug 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baszalmstra baszalmstra baszalmstra 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
@YeungOnion @baszalmstra