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

jellyfin-tui: init at v1.0.5 #374176

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

jellyfin-tui: init at v1.0.5 #374176

wants to merge 10 commits into from
+46 −0

Conversation

GKHWB
Copy link

@GKHWB GKHWB commented Jan 16, 2025

https://github.com/dhonus/jellyfin-tui

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jan 16, 2025
@Adda0
Copy link
Contributor

Adda0 commented Jan 18, 2025 •
edited
Loading

Hey. Thank you for contributing ❤️

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374176

x86_64-linux

✅ 1 package built:
  • jellyfin-tui

Reviewed points

✅ 18 | 🔴 5
  • package path fits guidelines (e.g., pkgs/by-name/so/some-package/package.nix for top-level packages).
  • Package name fits guidelines.
  • Package version fits guidelines.
  • Source is fetched using the appropriate function.
  • tag is used instead of rev for tag versions.
  • hash is used instead of sha256 for hashes.
  • Build time only dependencies are declared in nativeBuildInputs or its equivalent in the used builder.
  • The list of phases is not overridden.
  • When a phase (like installPhase) is overridden, it starts with runHook preInstall and ends with runHook postInstall.
  • Patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed.
  • Patches that are remotely available are fetched rather than vendored.
  • Package builds on x86_64-linux.
  • Executables tested on x86_64-linux.
  • meta.description is set and fits guidelines.
    • Descriptions should not contain complete sentences and should consist of phrases that fill the blank in " is a/an/the ____" (starting with a capital letter).
  • meta.license fits upstream license.
  • meta.platforms is set.
  • meta.maintainers is set.
  • meta.mainProgram is set, if applicable.
  • Whatever is needed is brought into scope instead of using top-level pkgs to make it easier to override.
  • General with lib; is not used.
  • If possible, update script is provided.
  • If possible, tests are provided.

Additional comments

Builds and runs, but I do not have a server setup to complete the initial configuration of the application further.

Adda0
Adda0 suggested changes Jan 18, 2025
View reviewed changes
pkgs/by-name/je/jellyfin-tui/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/je/jellyfin-tui/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/je/jellyfin-tui/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/je/jellyfin-tui/package.nix Show resolved Hide resolved
pkgs/by-name/je/jellyfin-tui/package.nix Outdated

rustPlatform.buildRustPackage rec {
pname = "jellyfin-tui";
version = "v1.0.5";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use v here. Use it only in the fetcher.

Suggested change
version = "v1.0.5";
version = "1.0.5";

Copy link
Author

Choose a reason for hiding this comment

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

Removed the v from version and concatenated v with version in the fetcher itself, not sure if that's the proper way to do it though.

Copy link
Contributor

@Adda0 Adda0 Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That is exactly it. Sorry, I should have clarified. Only, string interpolation is easier to type and much more concise.

pkgs/by-name/je/jellyfin-tui/package.nix Show resolved Hide resolved
@Adda0 Adda0 added 8.has: package (new) This PR adds a new package 2.status: needs-changes This PR needs changes by the author labels Jan 18, 2025
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/5110

@GKHWB GKHWB requested a review from Adda0 January 19, 2025 05:47
Adda0
Adda0 suggested changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@Adda0 Adda0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We are nearly there. Looking good.

Also, when you are done, squash all the commits into the initial one with init at.

And ideally, drop the v from version in the commit message and the PR title.

maintainers/maintainer-list.nix
Comment on lines +8464 to +8468
GKHWB = {
github = "GKHWB";
githubId = 68881230;
name = "GKHWB";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion, not necessary: When we are at it, would you be OK with adding some additional means of contacting you? Matrix, email? It is easier when it is needed than GitHub.

pkgs/by-name/je/jellyfin-tui/package.nix
Comment on lines +18 to +21
tag = lib.strings.concatStrings [
"v"
version
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tag = lib.strings.concatStrings [
"v"
version
];
tag = "v${version}";

Use string interpolation. It is much more concise.

pkgs/by-name/je/jellyfin-tui/package.nix

passthru.updateScript = nix-update-script { };

meta = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an application, add meta.mainProgram attribute.

@Adda0
Copy link
Contributor

Adda0 commented Jan 19, 2025 •
edited
Loading

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374176

x86_64-linux

✅ 1 package built:
  • jellyfin-tui

Reviewed points

✅ 25 | 🔴 1
  • All package paths fit the guidelines (e.g., pkgs/by-name/so/some-package/package.nix for top-level packages).
  • Package name fits guidelines.
  • Package version fits guidelines.
  • The packages sources are fetched using the appropriate functions (e.g., packages from GitHub should use fetchFromGitHub).
  • The package sources use mirror:// URLs when available.
  • tag is used instead of rev for tag versions.
  • hash is used instead of sha256 for hashes.
  • Build time only dependencies are declared in nativeBuildInputs or its equivalent in the used builder.
  • The list of phases is not overridden.
  • When a phase (like installPhase) is overridden, it starts with runHook preInstall and ends with runHook postInstall.
  • Patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed.
  • Patches that are remotely available are fetched rather than vendored.
  • Package builds on x86_64-linux.
  • Executables tested on x86_64-linux.
  • meta.description is set and fits guidelines.
    • Descriptions should not contain complete sentences and should consist of phrases that fill the blank in " is a/an/the ____" (starting with a capital letter).
  • meta.license fits upstream license.
  • meta.platforms should be set (or the package will not get binary substitutes).
  • Maintainers in meta.maintainers must be set.
    • This can be the package submitter or a community member that accepts taking up maintainership of the package.
  • The meta.mainProgram must be set if a main executable exists.
  • Whatever is needed is brought into scope instead of using top-level pkgs to make it easier to override.
  • General with lib; is not used.
  • If possible, update script is provided.
  • If possible, tests are provided.
  • The code contains no typos and all typos have been reported.

Additional comments

Builds and runs. Not tested beyond interactively creating the configuration (no Jellyfin server set up).

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

@Adda0 Adda0 Adda0 requested changes

Assignees
No one assigned
Labels
2.status: needs-changes This PR needs changes by the author 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GKHWB @Adda0 @nixos-discourse @NixOSInfra