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

Improve hacking.md and add clangd+bear to devshell #7433

Merged
merged 2 commits into from Feb 20, 2023

Conversation

yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Dec 9, 2022

Collaboration with @thufschmitt:

  • walked through the hacking and readme guide and tried to improve it somewhat
  • added clangd and bear to the devshell

Part of #7357

@edolstra
Copy link
Member

edolstra commented Dec 9, 2022

clang-tools is a very big dependency (1.3 GB) so I would prefer not to have that enabled by default. It would be okay for the clang-based stdenvs.

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Content-wise that's really good, thanks a lot! Left a few suggestions on formatting and wording.

In general, please use lists where there is a list of things, that's much easier to both scan and read. Always link to technical terms and commands, otherwise one has to look them up manually. Not everyone knows every detail of everything, and it gets hairy when the terms are not unique. Then readers won't be sure if they found the right thing.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
Comment on lines 6 to 5
This section provides some notes on how to hack on Nix. To get the
latest version of Nix from GitHub:

Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence and the code snippet that follows it should go above the flakes subsection. Sorry, can't select it properly due to GitHub's broken mobile UI.

doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
@thufschmitt
Copy link
Member

clang-tools is a very big dependency (1.3 GB) so I would prefer not to have that enabled by default. It would be okay for the clang-based stdenvs.

Totally agree.
Would it also be reasonable to switch to clang as the default stdenv for the shell given the better error messages that it gives and the availability of clangd?

@yorickvP yorickvP requested review from fricklerhandwerk and removed request for edolstra and thufschmitt December 16, 2022 15:36
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

This is taking a good shape!

Left another round of suggestions/minor fixes

flake.nix Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Deferring to @fricklerhandwerk

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-42/24204/1

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Judging from the inconsistencies that arose during editing, it may be better if we put the information on platforms and environments into separate subsections to have a single source of truth.

doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
@balsoft
Copy link
Member

balsoft commented Feb 1, 2023

@fricklerhandwerk I've addressed your comments (sorry I can't mark them as resolved). Would you mind taking another look?

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk 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 your patience with my nitpicky attention to detail. I really care about this being easy to maintain, and hope that you see why I made those suggestions, even if they appear like really many. Feel free to argue about them, I'm still trying to figure out a writing style and review procedure that scales for consumers and producers alike.

The contents are great and easy to follow, so all of those comments are about cosmetics – which is still important for smooth reading.

doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
@balsoft
Copy link
Member

balsoft commented Feb 10, 2023

@fricklerhandwerk addressed your feedback and rebased

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Last fix to fully sync up the two variants, otherwise good to merge from my side.

doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved
- Refer to current version in readme
- Split into flakes and non-flakes section
- Change order to move nix-build to the end, since people often start
  with it in the beginning.
- Use proper "Note" syntax
- Add notes about editor integration
- Move information about target platforms and stdenvs into separate
  sections

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Alexander Bantyev <alexander.bantyev@tweag.io>
Co-authored-by: Théophane Hufschmitt <theophane.hufschmitt@tweag.io>
@balsoft
Copy link
Member

balsoft commented Feb 13, 2023

Rebased&squashed, added co-author information.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-44/25546/1

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Let's merge this then! Thanks everyone

@thufschmitt thufschmitt merged commit 9a3f66d into NixOS:master Feb 20, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

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.

None yet

6 participants