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

emacs.pkgs.color-theme-solarized: convert to melpaBuild #279472

Closed
wants to merge 2 commits into from
Closed

emacs.pkgs.color-theme-solarized: convert to melpaBuild #279472

wants to merge 2 commits into from

Conversation

AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented Jan 7, 2024 •
edited
Loading

Description of changes

Part of #278925

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@AndersonTorres AndersonTorres marked this pull request as ready for review January 7, 2024 23:01
@AndersonTorres AndersonTorres marked this pull request as draft January 10, 2024 19:09
@AndersonTorres AndersonTorres force-pushed the color-theme-solarized branch 3 times, most recently from 854432f to 7c7e638 Compare March 31, 2024 20:30
@AndersonTorres AndersonTorres marked this pull request as ready for review March 31, 2024 20:30
@AndersonTorres
Copy link
Member Author

@samuelrivas
Since you are a very sporadic contributor and this package requires low maintenance effort, I took the initiative of removing you from its maintainers list.

Are you still interested in being a co-maintainer of this package?

@samuelrivas
Copy link
Contributor

Hi!

Thanks for taking care of this.

It is fine if you remove me, I would not claim that I am a very active maintainer :)

@AndersonTorres AndersonTorres marked this pull request as draft May 22, 2024 23:55
@AndersonTorres
emacs.pkgs.color-theme-solarized: convert to melpaBuild
acdd53c 
Also, gets rid of rec via lazy, explicitly-recursive set!
@AndersonTorres
emacs.pkgs.color-theme-solarized: remove samuelrivas from maintainers
510bff9 
This short package needs no more than one maintainer.
@AndersonTorres AndersonTorres marked this pull request as ready for review May 30, 2024 15:12
jian-lin
jian-lin reviewed May 31, 2024
View reviewed changes
.../applications/editors/emacs/elisp-packages/manual-packages/color-theme-solarized/default.nix
let
argset = {
pname = "color-theme-solarized";
ename = "color-theme-solarized";
Copy link
Contributor

Choose a reason for hiding this comment

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

ename defaults to pname

Suggested change
ename = "color-theme-solarized";

.../applications/editors/emacs/elisp-packages/manual-packages/color-theme-solarized/default.nix
src = fetchFromGitHub {
owner = "sellout";
repo = "emacs-color-theme-solarized";
rev = "b186e5d62d0b83cbf5cf38f7eb7a199dea9a3ee3";
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, the bump and the melpaBuild conversion should be in two separate commits.

.../applications/editors/emacs/elisp-packages/manual-packages/color-theme-solarized/default.nix
Comment on lines +20 to +23

packageRequires = [
color-theme
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed since sellout/emacs-color-theme-solarized@30dfe48.

Suggested change
packageRequires = [
color-theme
];

.../applications/editors/emacs/elisp-packages/manual-packages/color-theme-solarized/default.nix
license = licenses.mit;
maintainers = with maintainers; [ samuelrivas AndersonTorres ];
meta = {
homepage = "http://ethanschoonover.com/solarized";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the home page for this package. It's the home page for the Solarized theme.

And please use https.

Copy link
Member Author

Choose a reason for hiding this comment

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

THe HTTPS link fails sometimes. But OK, I will quote the GitHub link.

jian-lin
jian-lin reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

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

With #316107, this let argset = ... in ... refactor is not necessary.

@AndersonTorres
Copy link
Member Author

With #316107, this let argset = ... in ... refactor is not necessary.

Let's wait this land in master...

@AndersonTorres
Copy link
Member Author

Closing in favor of #325168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aleksanaa Aleksanaa Aleksanaa left review comments

@jian-lin jian-lin jian-lin left review comments

@adisbladis adisbladis Awaiting requested review from adisbladis adisbladis is a code owner

@piegamesde piegamesde Awaiting requested review from piegamesde

@Janik-Haag Janik-Haag Awaiting requested review from Janik-Haag

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: emacs 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndersonTorres @samuelrivas @Aleksanaa @jian-lin