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

ping: init 0.6.0 #56428

Merged
merged 2 commits into from
Mar 2, 2019
Merged

ping: init 0.6.0 #56428

merged 2 commits into from
Mar 2, 2019

Conversation

xiorcale
Copy link
Contributor

Motivation for this change

Since Pantheon desktop has recently been added to Master, adding some "curated" apps from Elementary AppCenter seems to be a good idea to complete it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 26, 2019
@worldofpeace worldofpeace added the 6.topic: pantheon The Pantheon desktop environment label Feb 27, 2019
worldofpeace
worldofpeace reviewed Feb 27, 2019
View reviewed changes
pkgs/development/web/ping/default.nix Outdated

src = fetchFromGitHub {
owner = "jeremyvaartjes";
repo = "ping";
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
repo = "ping";
repo = pname;

worldofpeace
worldofpeace reviewed Feb 27, 2019
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -9016,6 +9016,8 @@ in

phantomjs2 = libsForQt5.callPackage ../development/tools/phantomjs2 { };

ping = callPackages ../development/web/ping { };
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
ping = callPackages ../development/web/ping { };
ping = callPackage ../development/web/ping { };

Copy link
Contributor

@worldofpeace worldofpeace Feb 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also I don't think this belongs in development/web.

This is a gui application that is useful for debugging web API's, so maybe applications/networking.

Note that our organization in nixpkgs is kinda questionable at times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it in development/web because postman was there, which is more or less the same application than ping

Furthermore, it's a tool used for debugging API which are under development, so I think it makes sense to keep it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Manual says that for things under development should be

it’s used to support software development

And then goes on to mention libraries, compilers, and build-managers.
None on which will ever become a gui application.

You could say that an application like this would support software development, but so could an IDE, and they never should go under development.

I'd say that things like postman should be moved to a new place under applications/ and the closest thing we have currently is something like applications/networking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with you, I actually wanted to put the app under applications/networking before I discover that postman was under development/web

What should I do then ? Keeping consistency with the actual organization or move ping under applications/networking to better agree with the manual ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say applications/networking would be the best place currently.

We usually clean up the actual organization eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will move it to the right place then!

Also, I was wondering, wouldn't it be more convenient to have the curated apps from Elementary App Center organized under desktop/pantheon in a special directory for them? Because some of these apps seem to be tightly coupled with some part of the desktop (e.g. Wingpanel, ...).

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I will move it to the right place then!

Exclamation here confused me.

Also, I was wondering, wouldn't it be more convenient to have the curated apps from Elementary App Center organized under desktop/pantheon in a special directory for them? Because some of these apps seem to be tightly coupled with some part of the desktop (e.g. Wingpanel, ...).

I thought of this too, and I've decided that apps that specifically won't function without some part of the pantheon desktop can be organized there.
I've also patched granite so that they work outside pantheon (not needing wingpanel).
And even if they don't work I'd like them to default to not needing their desktop.
See: ac2546b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exclamation here confused me.

well, I was maybe a bit too enthusiastic that we found an agreement 😄

I had no idea about granite, but it makes sense yes. And the idea of the flag to default to not needing their desktop seems convenient too. Thanks for the details.

@worldofpeace
Copy link
Contributor

Can you also pull adding yourself to the maintainers-list into a separate commit?

maintainers: add kjuwi

@worldofpeace worldofpeace changed the title Ping init 0.6.0 ping: init 0.6.0 Feb 27, 2019
@GrahamcOfBorg GrahamcOfBorg removed the 6.topic: pantheon The Pantheon desktop environment label Feb 27, 2019
worldofpeace
worldofpeace suggested changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Everything's good, and I'll merge once the history is cleaned up to just having the two commits.

maintainers: add kjuvi
ping: init at 0.6.0

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
xiorcale added 2 commits March 2, 2019 09:57
@xiorcale
ping: init at 0.6.0
80dd94d 
ping: init at 0.6.0

Update pkgs/development/web/ping/default.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update pkgs/development/web/ping/default.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update pkgs/development/web/ping/default.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update pkgs/development/web/ping/default.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update pkgs/development/web/ping/default.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update maintainers/maintainer-list.nix

Co-Authored-By: Kjuvi <quentin.vaucher@protonmail.com>

Update maintainer-list.nix

ping: init at 0.6.0
@xiorcale
maintainers: add kjuvi
527d236 
fix missing glib

Fix missing import: hicolor-icon-theme

move ping under applications/networking

ping: init at 0.6.0

ping: init at 0.6.0

maintainers: add kjuvi
@worldofpeace worldofpeace merged commit 1560d5f into NixOS:master Mar 2, 2019
@worldofpeace
Copy link
Contributor

Hope you found the review informative @Kjuvi , and thank you for your contribution ✨

@xiorcale xiorcale deleted the ping-init-0.6.0 branch March 2, 2019 12:11
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 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
@xiorcale @worldofpeace @GrahamcOfBorg @Janik-Haag