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 from
Closed

Conversation

tuliocasagrande
Copy link
Contributor

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
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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

CONTRIBUTING.md Outdated
```sh
git clone https://github.com/coyim/coyim.git
cd coyim
export GOPATH=$HOME/code
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

CONTRIBUTING.md Outdated
5. Download go dependencies:
```
make deps-dev
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably: 'Download the project dependencies:'

@claucece
Copy link
Contributor

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
Copy link
Contributor Author

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
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
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants