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

htmlbeautifier: init at 1.3.1 #87928

Open
wants to merge 2 commits into
base: master
from
Open

Conversation

@mveytsman
Copy link
Contributor

mveytsman commented May 16, 2020

Motivation for this change

Adds htmlbeautifier an HTML formatter with Ruby/ERB support.

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.
@doronbehar
Copy link
Contributor

doronbehar commented May 16, 2020

Don't know why, but please note that there's a CI evaluation error.

@mveytsman mveytsman force-pushed the mveytsman:htmlbeautifier branch 2 times, most recently from 0d33666 to bf6dcff May 16, 2020
email = "maxim@ontoillogicmal.com";
github = "mvetsman";
githubId = 34720;
name = "Max Vetsman";
Comment on lines 5416 to 5419

This comment has been minimized.

Copy link
@shazow

shazow May 16, 2020

Contributor
Suggested change
email = "maxim@ontoillogicmal.com";
github = "mvetsman";
githubId = 34720;
name = "Max Vetsman";
email = "maxim@ontoillogical.com";
github = "mveytsman";
githubId = 34720;
name = "Max Veytsman";

This comment has been minimized.

Copy link
@mveytsman

mveytsman May 16, 2020

Author Contributor

<3 thanks @shazow

@mveytsman mveytsman force-pushed the mveytsman:htmlbeautifier branch from bf6dcff to 0d85897 May 16, 2020
@@ -5412,6 +5412,12 @@
githubId = 5047140;
name = "Victor Collod";
};
mveytsman = {

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 16, 2020

Contributor

You also need to make sure your PR eventually has 2 commits:

maintainers: add mveytsman
htmlbeautifier: init at 1.3.1

This comment has been minimized.

Copy link
@mveytsman

mveytsman May 16, 2020

Author Contributor

Thanks, that wasn't clear to me, I amended the history accordingly

exes = [ "htmlbeautifier" ];

# toString prevents the update script from being copied into the nix store
passthru.updateScript = toString ./update;

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 16, 2020

Contributor

Question: Do all bundlerApp packages use such an update script? The script it self seems trivial that I doubt there is no generic way of doing so. It seems there is something related to that according to https://nixos.org/nixpkgs/manual/#sec-language-ruby

This comment has been minimized.

Copy link
@mveytsman

mveytsman May 16, 2020

Author Contributor

I found similar ones in cocoapods but I see some that don't have it, e.g ruby-zoom and solargraph.

Do you think I should remove it?

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 16, 2020

Contributor

I think cocoapods' update script is a just a bit "non-trivial" because it also handles a Gemfile-beta file for as far as I can see.

Can you try:

  passthru.updateScript = bundlerUpdateScript "htmlbeautifier";

You should be able to check if this works with:

nix-shell maintainers/scripts/update.nix --argstr package htmlbeautifier

See https://nixos.org/nixpkgs/manual/#var-passthru-updateScript

This comment has been minimized.

Copy link
@mveytsman

mveytsman May 19, 2020

Author Contributor

@doronbehar ah amazing! thanks for clarifying. I changed it as you suggested and it seems to work.

This comment has been minimized.

Copy link
@kimat

kimat May 22, 2020

Contributor

Do all bundlerApp packages use such an update script?

Most "simple ruby packages" do, since: #64822.

@mveytsman mveytsman force-pushed the mveytsman:htmlbeautifier branch from 0d85897 to b42acff May 16, 2020
@mveytsman mveytsman force-pushed the mveytsman:htmlbeautifier branch from b42acff to 01fc8d7 May 19, 2020
Copy link
Contributor

doronbehar left a comment

LGTM but I'm no experienced in reviewing bundler PRs. Maybe @aanderse will have something more to add.

@aanderse
Copy link
Contributor

aanderse commented May 20, 2020

Sorry, I can't be of help... I'm not a ruby guy 😞

@mveytsman
Copy link
Contributor Author

mveytsman commented May 20, 2020

No worries!

Do you know if there's anyone else we should ask for a 👀 on this PR?

@kimat
Copy link
Contributor

kimat commented May 22, 2020

I verified that:

@aanderse
Copy link
Contributor

aanderse commented Jun 7, 2020

Well... I hate to do it, but I'll bug @manveru for a quick review here.

Copy link
Contributor

manveru left a comment

Two small issues, looks good otherwise.

description = "A normaliser/beautifier for HTML that also understands embedded Ruby, ideal for tidying up Rails templates";
homepage = "https://github.com/threedaymonk/htmlbeautifier";
license = licenses.mit;
platforms = platforms.linux;

This comment has been minimized.

Copy link
@manveru

manveru Jun 7, 2020

Contributor

This should probably be ruby.meta.platforms, since it's pure Ruby code and should run everywhere Ruby does?

gemfile = ./Gemfile;
lockfile = ./Gemfile.lock;
gemset = ./gemset.nix;
Comment on lines +6 to +8

This comment has been minimized.

Copy link
@manveru

manveru Jun 7, 2020

Contributor

This can be expressed as gemdir = ./.;

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
You can’t perform that action at this time.