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

staticjinja: enable tests; lift to top level #104401

Merged
merged 2 commits into from Jan 11, 2021

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Nov 20, 2020

Motivation for this change

Lifted to the top level because most users will probably use the cli tool

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.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 104401 run on x86_64-linux 1

2 packages built:
  • python37Packages.staticjinja
  • staticjinja (python38Packages.staticjinja)

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 104401 run on x86_64-darwin 1

2 packages built:
  • python37Packages.staticjinja
  • staticjinja (python38Packages.staticjinja)

@FRidh
Copy link
Member

FRidh commented Dec 2, 2020

Lifted to the top level because most users will probably use the cli tool

however, the tool will import the Python code one wrote which imports other modules.

I think its a bad idea to expose this one as top-level attribute and think that it should always be considered as part of a Python environment that has all the modules that one uses in a staticjinja project. Or are you saying one never uses additional Python libraries with staticjinja?

@fgaz
Copy link
Member Author

fgaz commented Dec 2, 2020

@FRidh I'm saying there's two ways of using this library/program

  1. By using it as a dependency in a python environment.
  2. By just using the command line tool (staticjinja build), like in most site generators. This might as well be in a larger python project, but in this case staticjinja is treated only as an executable, not as a library. (this is the only way I personally used this project)

Having the executable at the top level facilitates the second, simpler, way

edit: to be clear, in (2), one does not write python code to generate the site, but only jinja templates, which afaik can't import python code by themselves

@FRidh
Copy link
Member

FRidh commented Dec 2, 2020

By just using the command line tool (staticjinja build), like in most site generators. This might as well be in a larger python project, but in this case staticjinja is treated only as an executable, not as a library. (this is the only way I personally used this project)

I understand, but would you argue that one typically does so without any additional Python libraries being used aside from those required by staticjinja?

@fgaz
Copy link
Member Author

fgaz commented Dec 2, 2020

I'm not sure I understand, could you make an example of using some python library together with staticjinja (the executable), and having the second interfere with the first?

I typically just do something like this (taken from the README):

$ mkdir templates
$ vim templates/index.html
$ staticjinja watch
Building index.html...
Templates built.
Watching 'templates' for changes...
Press Ctrl+C to stop.

Basically I just use it as a simple way to run a template engine. As far as I can see there isn't even a way to use additional python libraries when running it like this.

@FRidh
Copy link
Member

FRidh commented Dec 2, 2020

Okay, I see. I mostly skimmed through the advanced section, thinking that becomes pretty essential considering the basic functionality is so limited.

@fgaz
Copy link
Member Author

fgaz commented Dec 2, 2020

It depends. If you don't have to load external data (ex. for building simple sites, or for prototyping or reworking a bunch of existing pages) it's surprisingly powerful. If you prefer I can remove the second commit though.

@fgaz
Copy link
Member Author

fgaz commented Jan 1, 2021

Ping

@fgaz
Copy link
Member Author

fgaz commented Jan 1, 2021

Oh wait, I see that @FRidh updated it independently in 722c434. Let me rebase...

@fgaz fgaz changed the title staticjinja: 0.3.5 -> 0.4.0; lift to top level staticjinja: enable tests; lift to top level Jan 1, 2021
and:
* fetch from github (tests on pypi get somehow disabled)
* formatting
Most user will probably use the cli tool
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 104401 run on x86_64-darwin 1

3 packages built:
  • python37Packages.staticjinja
  • staticjinja (python38Packages.staticjinja)
  • python39Packages.staticjinja

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 104401 run on x86_64-linux 1

3 packages built:
  • python37Packages.staticjinja
  • staticjinja (python38Packages.staticjinja)
  • python39Packages.staticjinja

@SuperSandro2000 SuperSandro2000 merged commit d8165ca into NixOS:master Jan 11, 2021
@fgaz fgaz deleted the staticjinja/0.4.0 branch January 11, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants