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

pip guides, docs devshell fix #1013

Merged
merged 19 commits into from
Jul 29, 2024
Merged

pip guides, docs devshell fix #1013

merged 19 commits into from
Jul 29, 2024

Conversation

phaer
Copy link
Member

@phaer phaer commented Jul 11, 2024

  • pillow example
  • python-local-development example
  • devshell with editables
  • editables options docs
  • pdm example & comparision
  • more elaborate flattenDependencies option docs

@phaer phaer requested a review from DavHau July 11, 2024 12:25
@phaer
Copy link
Member Author

phaer commented Jul 11, 2024

@purepani For some reason, I can't select you as reviewer here, but this might be of interest to you :)

Copy link
Contributor

@purepani purepani left a comment

Choose a reason for hiding this comment

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

I'm generally of the opinion that "getting started" guides should have a super simple example that people generally copy and build off of, so maybe we could simplify the "mytool" example a bit, though I'd maybe just push this as it is for now since that is fixable later.

Also I'm not exactly sure what the name of the second example is. I remember changing it from "my-tool" to "mytool" or something since the hyphen caused issues, so we should make sure that is consistent.

docs/src/guides/pip.md Outdated Show resolved Hide resolved
docs/src/guides/pip.md Outdated Show resolved Hide resolved
docs/src/guides/pip.md Outdated Show resolved Hide resolved
docs/src/guides/pip.md Show resolved Hide resolved
Comment on lines +194 to +201
src = lib.cleanSourceWith { # (5)
src = lib.cleanSource ./.;
filter = name: type:
!(builtins.any (x: x) [
(lib.hasSuffix ".nix" name)
(lib.hasPrefix "." (builtins.baseNameOf name))
(lib.hasSuffix "flake.lock" name)
]);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should include the filtering of the source in this example, since it's not really needed in a "getting started" guide, and most people would probably take this example and copy paste it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a good question. The exaple here is, as you probably noticed, a verbatim copy of the python-local-development one in ./examples. If we remove it here, I'd prefer to remove it over there as well for consistency.

On one hand i can see that it opens too many topics at once for people, on the other hand getting an idea of whats copied to the nix store when is important to have any change of understanding why a rebuild happens - even more so with devshells.

I see if i manage to do a better explanation on that, or otherwise remove it for now.

@phaer phaer force-pushed the guides branch 2 times, most recently from 343ff80 to 60582c3 Compare July 28, 2024 20:41
@phaer phaer marked this pull request as ready for review July 29, 2024 10:29
Copy link
Member

@DavHau DavHau left a comment

Choose a reason for hiding this comment

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

Looks good, except the one thing I commented on.

docs/theme/partials/toc-item.html Outdated Show resolved Hide resolved
@phaer phaer merged commit ece62d6 into nix-community:main Jul 29, 2024
99 checks passed
@phaer phaer deleted the guides branch July 29, 2024 11:25

We recommend reading our [Getting Started](./getting-started.md) guide first if you have not done so yet!

this guide we are going to take a look at two annotated examples using the [pip module](../reference/pip/index.md):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hate to point this out right after you pushed, but I think this missing an "In" at the beginning of the sentence!

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, right. No worries, I'll fix it with the next PR, probably later "today" (I am in CEST).

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