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

dracula-theme: 1.3.0 -> 2.0 and rename ant-dracula to dracula-theme #99704

Merged
merged 2 commits into from Oct 11, 2020

Conversation

@Vonfry
Copy link
Member

@Vonfry Vonfry commented Oct 6, 2020

Motivation for this change

Update package and rename it due to the upstream repo has been renamed.

Things done
  • 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.

cc: @alexarice
fixes: #97655

@Vonfry Vonfry force-pushed the update/dracula branch from 3e80250 to 108e0de Oct 6, 2020
Copy link
Contributor

@alexarice alexarice left a comment

nix-review passes. Have left some comments

@@ -9088,6 +9088,12 @@
githubId = 508305;
name = "Jaroslavas Pocepko";
};
vonfry = {
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

Can the change to maintainer-list.nix be in a separate commit titled "maintainers: add vonfry"

};

propagatedUserEnvPkgs = [
# gtk-engine-murrine
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

If this isn't needed this shouldn't be here

Copy link
Contributor

@alexarice alexarice Oct 6, 2020

Think you've already removed this

@@ -18767,7 +18767,7 @@ in

ant-bloody-theme = callPackage ../data/themes/ant-theme/ant-bloody.nix { };

ant-dracula-theme = callPackage ../data/themes/ant-theme/ant-dracula.nix { };
dracula-theme = callPackage ../data/themes/dracula-theme { };
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

This will break existing configurations. Maybe there should be an error thrown when using the old package that tells you to use the new one and to change the theme name in your config

Copy link
Contributor

@kevincox kevincox Oct 11, 2020

Yes, typically you will log a warning for at least a couple of months.

Example:

traceXMLVal = x:

Copy link
Contributor

@alexarice alexarice Oct 11, 2020

I believe the alias should add an appropriate warning

@Vonfry Vonfry force-pushed the update/dracula branch 3 times, most recently from 8774780 to 710a9b7 Oct 6, 2020
Copy link
Contributor

@alexarice alexarice left a comment

Is this the sort of change that should have a release note?

@@ -738,4 +738,7 @@ mapAliases ({
/* Cleanup before 21.03 */
riot-desktop = throw "riot-desktop is now element-desktop!";
riot-web = throw "riot-web is now element-web";

ant-dracula-theme = throw "ant-dracula-theme is now dracula-theme";
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

Can this mention that the theme name has changed. For example I will have to change my config here

@Vonfry Vonfry force-pushed the update/dracula branch from 710a9b7 to 35d3207 Oct 6, 2020
Copy link
Contributor

@alexarice alexarice left a comment

Looks good to me, Thanks for doing this fix, I wasn't even aware there had been an update.
I'll look into release notes, but other than that nix-review passes and looks good to go

@ofborg ofborg bot requested a review from alexarice Oct 6, 2020
themeName = "Dracula";
version = "2.0";
in
stdenv.mkDerivation rec {
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation {

Sorry, small change I missed

@ofborg ofborg bot requested a review from alexarice Oct 6, 2020
@Vonfry Vonfry force-pushed the update/dracula branch from 35d3207 to ad9368b Oct 6, 2020
Copy link
Contributor

@alexarice alexarice left a comment

Asked on irc and I don't think there's any need for release notes so should be good as is

@ofborg ofborg bot requested a review from alexarice Oct 6, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 11, 2020

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/244


propagatedUserEnvPkgs = [
gtk-engine-murrine
];
Copy link
Contributor

@romildo romildo Oct 11, 2020

Why is gtk-engine-murrine removed? It seems that the gtk2 theme still depends on this engine.

Copy link
Member Author

@Vonfry Vonfry Oct 11, 2020

Sorry, I checked the source again. It uses this in gnome-terminal, which I don't use. I have added it back.

@Vonfry Vonfry force-pushed the update/dracula branch from ad9368b to 0f1be68 Oct 11, 2020
@romildo romildo merged commit e2064ae into NixOS:master Oct 11, 2020
19 checks passed
@Vonfry Vonfry deleted the update/dracula branch Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment