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

Remove periods from the end of package descriptions #97835

Merged
merged 1 commit into from Oct 17, 2020

Conversation

@siraben
Copy link
Member

@siraben siraben commented Sep 12, 2020

Motivation for this change

Addresses #97685

Things done

Got the list of packages to fix with a Nix script (see #97685), then went over the paths with an awk file;

{ system("sed --in-place='' -re 's#(description\\s*=\\s*\".+)\\.\"#\\1\"#' " $0)}
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@siraben siraben force-pushed the desc-period-fix branch from def7408 to a389064 Sep 12, 2020
@siraben siraben requested a review from cdepillabout Sep 12, 2020
@ofborg ofborg bot removed the 6.topic: haskell label Sep 12, 2020
@teto
Copy link
Contributor

@teto teto commented Sep 12, 2020

doesn't mean much if we can't enforce this rule via ofborg. Also some of these descriptions are generated so we would need to fix those descriptions as well. Not worth the hassle IMO.

@siraben
Copy link
Member Author

@siraben siraben commented Sep 12, 2020

We can put this in ofborg (see #97685), but for now, it would be good to fix up the description of hundreds of packages.

@siraben
Copy link
Member Author

@siraben siraben commented Sep 12, 2020

I'll remove the edits to the autogenerated packages.

@siraben
Copy link
Member Author

@siraben siraben commented Oct 11, 2020

@worldofpeace Should I resolve the merge conflicts and have this be merged? The plan is for @GrahamcOfBorg to check this for new PRs in the future.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Oct 11, 2020

Sure, let's try to merge it quickly to avoid any more conflicts.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Oct 11, 2020

@worldofpeace Should I resolve the merge conflicts and have this be merged? The plan is for @GrahamcOfBorg to check this for new PRs in the future.

I think we're actually considering moving everything into github checks.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Oct 11, 2020

I think we're actually considering moving everything into github checks.

I'm okay with this, scales much better; and don't have to bug graham all the time

@siraben siraben force-pushed the desc-period-fix branch 2 times, most recently from bb4a76b to 467e6d2 Oct 11, 2020
@siraben
Copy link
Member Author

@siraben siraben commented Oct 11, 2020

For future reference,

Nix script to fetch paths to derivations whose descriptions end with period
let
  pkgs = import ./../../default.nix {};

  safeEval = e: let result = builtins.tryEval e;
                in
                  if result.success
                  then result.value
                  else [ ];

  packagesWith = cond:
    pkgs.lib.concatLists (pkgs.lib.mapAttrsToList
      (name: pkg: safeEval
        (if pkgs.lib.isDerivation pkg && cond name pkg
         then [ (builtins.elemAt (pkgs.lib.splitString ":" pkg.meta.position) 0) ]
         else [ ]))
      pkgs);

  hasDesc = pkg: builtins.hasAttr "description" pkg.meta;
  lastChar = x: builtins.substring (builtins.stringLength x - 1) (-1) x;
  report = desc: l:
    builtins.trace (pkgs.lib.concatStringsSep "\n" l) "";
in
{
  periodDesc =
    report "have a period at the end of their description"
      (packagesWith
        (name: pkg:
          if hasDesc pkg && builtins.stringLength pkg.meta.description != 0
          then lastChar pkg.meta.description == "."
          else false));
}

The output of the script is fed into an awk file with contents

{ system("sed --in-place='' -re 's#(description\\s*=\\s*\".+)\\.\"#\\1\"#' " $0)}

Finally, I went through the changed files to see which ones had a delta greater than 2, indicating it was probably a package collection (Haskell/Node/Lua packages came up).

@siraben siraben force-pushed the desc-period-fix branch from 6e43139 to c1cbbba Oct 17, 2020
@siraben
Copy link
Member Author

@siraben siraben commented Oct 17, 2020

@jonringer @worldofpeace Should be ready to merge.

@@ -22,14 +22,14 @@ stdenv.mkDerivation rec {

installPhase = ''
mkdir -p $out/bin $out/share/doc/bliss $out/lib $out/include/bliss
mv bliss $out/bin
mv bliss $out/bin
Copy link
Contributor

@jonringer jonringer Oct 17, 2020

oh, the rebuild is due to whitespace, for the check.

Copy link
Contributor

@jonringer jonringer left a comment

LGTM

Result of nixpkgs-review pr 97835 1

1 package blacklisted:
  • tests.nixos-functions.nixos-test
4 packages built:
  • bliss
  • documentation-highlighter
  • polymake
  • trilium-desktop

@jonringer jonringer merged commit 683a87d into NixOS:master Oct 17, 2020
17 checks passed
@siraben siraben deleted the desc-period-fix branch Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants