search

guillermomuntaner / Burritos

A collection of Swift Property Wrappers (formerly "Property Delegates")
MIT License
1.33k stars 43 forks source link

Adding @EnvironmentVariable wrapper #7

Closed LucianoPAlmeida closed 5 years ago

LucianoPAlmeida commented 5 years ago

Hey :)) This is a simple one, just adds a property wrapper to get and set the system's env vars.

guillermomuntaner commented 5 years ago

Hey @LucianoPAlmeida, thanks for contributing.

It looks already really good to me, just want to discuss couple more ideas:

  1. Do you think is useful to expose the setter? It could be read only too.
  2. I understand you went with the Foundation API for reading since it is available and went lower level for writing. I am just wondering if it would be nicer to use same API for both getter & setter. By using getenv we could drop Foundation and use Glibc to make this available also in Linux.

Looking forward to hearing your thoughts. Regards.

LucianoPAlmeida commented 5 years ago

Hey @guillermomuntaner :))

  1. Do you think is useful to expose the setter? It could be read-only too.

I'm not sure if it's useful, personally most of the times I just read them. But I thought will not hurt to leave a set as an option since is very simple to implement. But I could make read-only no problem :)

  1. I understand you went with the Foundation API for reading since it is available and went lower level for writing. I am just wondering if it would be nicer to use the same API for both getter & setter. By using getenv we could drop Foundation and use Glibc to make this available also in Linux.

I think this is available on CoreLibs Foundation too(here), so use this API will work on Linux as well. I just choose that for simplicity, but now that you mention maybe we use the low level on getter too for consistency. I'm making the changes 👍

guillermomuntaner commented 5 years ago

Yeah I saw the port of ProcessInfo in CoreLibs Foundation, but I am really unaware of the status of the whole project and other platform support in general. This can be worked out later since lots of other pieces missing for Linux support anyways.

One last request though; could you update the cocoapods podspec with a new submodule? I forgot to add that one to the contrib guidelines, sorry about that.

Thanks a lot!

LucianoPAlmeida commented 5 years ago

Yeah I saw the port of ProcessInfo in CoreLibs Foundation, but I am really unaware of the status of the whole project and other platform support in general. This can be worked out later since lots of other pieces missing for Linux support anyways.

Sure :))

One last request though; could you update the cocoapods podspec with a new submodule? I forgot to add that one to the contrib guidelines, sorry about that.

Done 👍

guillermomuntaner commented 5 years ago

Thanks a lot :)