New functions for the System module #230

Merged
merged 13 commits into from Apr 10, 2012

Conversation

Projects
None yet
4 participants
Member

alco commented Apr 4, 2012

Not sure how to deal with the "binary vs charlist" issue. I tried to reach a balance with adding binary support for some functions. put_env would require 4 clauses to accept all pairs "binary-list", "list-binary", "binary-binary", and "list-list", so I didn't do it for this function.

@josevalim josevalim commented on an outdated diff Apr 4, 2012

lib/system.ex
@@ -65,6 +65,69 @@ defmodule System do
end
@doc """
+ Executes `command` in a command shell of the target OS, captures the standard
+ output of the command and returns this result as a string.
+
+ `command` can be an atom, a charlist or a binary.
+ """
+ def cmd(command) when is_binary(command) do
+ cmd binary_to_list(command)
@josevalim

josevalim Apr 4, 2012

Owner

We should go with Erlang philosophy here. If the argument is a binary, we return a binary. If a char list, we return a char list.

Owner

josevalim commented Apr 4, 2012

I see your point about binary and char list. In general, Erlang accepts both arguments and the result is given according to the argument (if a binary is given, we return a binary). However, supporting both variants in many functions will become rather annoying.

Member

alco commented Apr 4, 2012

I'm inclined to revert to simply allowing charlists for the functions in this PR, like Erlang does. When people start asking for binaries, we can always review the decision.

Owner

josevalim commented Apr 4, 2012

I disagree because Elixir actually favors binaries over char lists. So we could make it otherwise, to not accept char lists, but for sure we need to accept binaries (the Erlang community is slowing catching up in adding binaries to its libraries but it is becoming more and more frequent, for example, Cowboy and Riak are very binary driven).

Member

alco commented Apr 4, 2012

Ok, I don't have enough familiarity with the issue, so I'll go with your argument. Should I add the binary variants of put_env/2 in this PR?

Owner

josevalim commented Apr 4, 2012

We can just go with def put_env(key, value), do: :os.putenv(to_char_list(key), to_char_list(value))

Member

alco commented Apr 4, 2012

I've added a few more updates.

Member

alco commented Apr 5, 2012

I have updated git version and date queries and rebased on master. Can we get someone to test this on windows? /cc @yksirius

Simply pull my branch over at https://github.com/alco/elixir/tree/system-module and try to build it. Then check the result with

bin/elixir.bat -e "IO.inspect System.build_info"

or how it's usually done on Windows...

Contributor

EricHripko commented Apr 5, 2012

Hello!

Here is a result:

e:__AlcoTest\elixir\bin>elixir.bat -e IO.inspect(System.build_info)
{"0.9.0.dev","16f95060c75ee92659d7f0439d81f6b8c8871c8d","Thu, 05 Apr 2012 14:13:
36 GMT"}

Member

alco commented Apr 5, 2012

Great! Thanks.

I think it's ready to be merged.

Owner

josevalim commented Apr 5, 2012

Great, thanks a lot. Finally, can we have tests where possible? (at least get_env, put_env and cmd)

Member

alco commented Apr 5, 2012

ping

@josevalim josevalim added a commit that referenced this pull request Apr 10, 2012

@josevalim josevalim Merge pull request #230 from alco/system-module
New functions for the System module
58d45c1

@josevalim josevalim merged commit 58d45c1 into elixir-lang:master Apr 10, 2012

Contributor

yrashk commented on lib/system.ex in 9137e30 Jun 3, 2012

How about using simple :os.cmd/1 here?

Member

alco replied Jun 3, 2012

os.cmd returns an error as its output if git is not installed on the system. You can see that I had used it and then switched to a port.

Contributor

yrashk replied Jun 3, 2012

didn't see that, sorry — was just skimming through. but I'd actually do os:cmd anyway but just do os:find_executable before that. How about that?

Member

alco replied Jun 3, 2012

Sounds nice, that would be a lot simpler than ports. What bothers me is whether this functionality is useful at all. For instance, when installing via homebrew, the commit sha-1 is not embedded within the executable.

Contributor

yrashk replied Jun 3, 2012

That's a good point, too. But if we are to keep this code, I'd strongly recommend switching to os:find_executable/1 and os:cmd/1. That would just trim down the code tremendously.

Member

alco replied Jun 4, 2012

I agree. Contributions are welcome ;)

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