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

Improve installation instructions #452

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tuliocasagrande
Copy link
Contributor

tuliocasagrande commented Oct 9, 2017

I changed a little bit the installation instructions to address some issues I faced when installing for the first time.

I still cannot pass all the tests, so I might updated this PR later.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage remained the same at 31.258% when pulling 82c6b92 on update-instructions into 736acd3 on master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage remained the same at 31.258% when pulling 82c6b92 on update-instructions into 736acd3 on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage remained the same at 31.258% when pulling 82c6b92 on update-instructions into 736acd3 on master.

```sh
git clone https://github.com/coyim/coyim.git
cd coyim
export GOPATH=$HOME/code

This comment has been minimized.

@claucece

claucece Oct 11, 2017

Member

maybe something like: export GOPATH=$HOME/your-name might be clearer.

This comment has been minimized.

@juniorz

juniorz Oct 11, 2017

Member

Can I ask why is this useful? I'm against explaining people how to setup their language environment: it changes frequently and it's the entry-level knowledge required to contribute.

The particular case of gopath is well documented and dont really is mandatory to have an exported 'GOPATH` variable (I dont):

$ go help gopath
The Go path is used to resolve import statements.
It is implemented by and documented in the go/build package.

The GOPATH environment variable lists places to look for Go code.
On Unix, the value is a colon-separated string.
On Windows, the value is a semicolon-separated string.
On Plan 9, the value is a list.

If the environment variable is unset, GOPATH defaults
to a subdirectory named "go" in the user's home directory
($HOME/go on Unix, %USERPROFILE%\go on Windows),
unless that directory holds a Go distribution.
Run "go env GOPATH" to see the current GOPATH.

See https://golang.org/wiki/SettingGOPATH to set a custom GOPATH.

Is there anything in our project that mandates a particular setup that diverges from the language default environment? If so, we document. Otherwise, we simply outline the workflow here.

This comment has been minimized.

@juniorz

juniorz Oct 11, 2017

Member

I believe the problem is really that our Makefile is "broken". go get github.com/coyim/coyim should just work but it doesn't.

This comment has been minimized.

@tuliocasagrande

tuliocasagrande Oct 11, 2017

Author Contributor

hey @juniorz, I agree with you that environment setups change frequently and don't necessary should be in projects instructions.

Do you think the last commit, which states just a reminder for GOPATH and PATH, is OK or should be removed?

5. Download go dependencies:
```
make deps-dev
```

This comment has been minimized.

@claucece

claucece Oct 11, 2017

Member

This is probably: 'Download the project dependencies:'

@claucece

This comment has been minimized.

Copy link
Member

claucece commented Oct 11, 2017

If you want, this can be merged directly. As it is a markdown project file, it does not really need to
pass the tests.

Thanks for this!

@tuliocasagrande

This comment has been minimized.

Copy link
Contributor Author

tuliocasagrande commented Oct 11, 2017

hey @claucece, thanks for the feedback.
What I meant about the tests is that the instructions might still be inaccurate, having room for more improvements.

However, it seems the failing test is not related with my installation, but with my local certificates. Therefore, I think we can merge this.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage decreased (-0.05%) to 31.207% when pulling 25bdf9f on update-instructions into 736acd3 on master.

@tuliocasagrande

This comment has been minimized.

Copy link
Contributor Author

tuliocasagrande commented Oct 13, 2017

Hey @juniorz, I saw that your commit 8d11889 added the highest value that this PR proposed to do, which was changing the instructions from git clone to go get.

Considering that and taking into account the desire to not baby step new developers, I thing we can close/remove this PR.

@juniorz juniorz closed this Oct 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.