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

Improvements in the TUTORIAL.md file #3435

Merged
merged 15 commits into from
May 15, 2019
Merged

Improvements in the TUTORIAL.md file #3435

merged 15 commits into from
May 15, 2019

Conversation

JosepBernad
Copy link
Contributor

@JosepBernad JosepBernad commented May 7, 2019

While I was trying to install shields for the first time, I ran into a problem that I solved.
I've added the solution in the doc/TUTORIAL.md file in order to ease the process for the newbies like me.
Source of the solution: https://medium.com/andrewmmc-io/node-js-error-getaddrinfo-enotfound-localhost-b7ee35e1bb60

@shields-ci
Copy link

shields-ci commented May 7, 2019

Messages
📖 ✨ Thanks for your contribution to Shields, @JosepBernad!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against fadfd9e

@calebcartwright calebcartwright added the documentation Developer and end-user documentation label May 7, 2019
@calebcartwright
Copy link
Member

Thanks for this! Do you think it might be easier to just have the docs suggest trying to hit 127.0.0.1:3000 instead?

@JosepBernad
Copy link
Contributor Author

What do you think now @calebcartwright?

@paulmelnikow
Copy link
Member

HI! Did you run into this issue? What OS was that on?

Usually OS's will include this alias by default; it's rare that I've seen something like this happen.

I'm with Caleb… not sure I'd want to direct new users to edit their /etc/hosts file 🙈

@calebcartwright
Copy link
Member

I'd echo @paulmelnikow question about your OS if you experienced this issue. We want to make sure anyone can get Shields running on their local machine regardless of their OS, without having to manually edit the hosts file.

If there's a particular OS/environment where our npm start script won't work unless the hosts file is manually updated, then I'd be inclined to review our scripts/config to see if we can address that instead.

@JosepBernad
Copy link
Contributor Author

JosepBernad commented May 8, 2019

So, do you guys think that the best option is to replace localhost:3000 with 127.0.0.1:3000?

@paulmelnikow
Copy link
Member

Can you say more about how you encountered this issue? What OS are you using?

@JosepBernad
Copy link
Contributor Author

I'm using macOS 10.12.6 (Sierra), node 10.15.3 and npm 6.4.1.
I was just following the TUTORIAL.md when I had this problem:
Error: getaddrinfo ENOTFOUND localhost.
I researched it and found that the system can't find localhost. So, I decided to add the explanation to add localhost manually.
Some folks told me that that's not the best way to do it so I think that the best way to do it is accessing straight to the IP address (instead of the localhost address).

What do you think, @paulmelnikow?

@JosepBernad JosepBernad changed the title Small improvement in the TUTORIAL file Improvements in the TUTORIAL.md file May 9, 2019
@JosepBernad
Copy link
Contributor Author

Also, I've just added the method in the first example, because it didn't compile:
static get category() {
return 'build';
}

@paulmelnikow
Copy link
Member

Thanks for adding the category!

Re /etc/hosts:

Here's an example of this issue reported on another project: angular/angular-cli#2227. I'm gleaning from that issue that this line is in /etc/hosts by default which certainly is also my expectation. So I think this issue occurs when the original contents of /etc/hosts is manually changed. Probably it is better if most users do not edit /etc/hosts for exactly this reason.

We could use 127.0.0.1 instead, though again, I'm not aware of any systems that don't have this mapping by default.

We shouldn't cover every eventuality in our tutorial and I don't want to give people the impression that they should be editing /etc/hosts (which seems to be what caused this issue). I suppose we could add a troubleshooting section, and maybe link out to that issue. (particularly angular/angular-cli#2227 (comment))

@JosepBernad
Copy link
Contributor Author

Nice, let me modify the file it with your proposal.

@JosepBernad
Copy link
Contributor Author

I've updated the TUTORIAL.md file with your proposals. Could someone review it in order to close it?
Thanks for your help. I'm really proud of my very first issue!

doc/TUTORIAL.md Outdated Show resolved Hide resolved
Co-Authored-By: Caleb Cartwright <calebcartwright@users.noreply.github.com>
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

👍

@calebcartwright
Copy link
Member

This LGTM now. @paulmelnikow any other thoughts on this one?

@JosepBernad
Copy link
Contributor Author

I've just added the explanation for both get category() functions.

@JosepBernad
Copy link
Contributor Author

Can we close this PR or is there anything else to do?

@calebcartwright
Copy link
Member

Can we close this PR or is there anything else to do?

Each PR has to be reviewed, and approved by at least one member of the maintainer team before it can be merged. Even after a PR has been approved, additional changes (like the one you made a few hours ago) restart the process.

We'll take a look at your latest commit when we can and potentially provide additional feedback and/or request changes.

@JosepBernad
Copy link
Contributor Author

Thanks @calebcartwrigh! I I didn't want to seem to hurry, sorry if I did!

@calebcartwright
Copy link
Member

No worries!

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Looks good to me! Suggested one minor tweak.

doc/TUTORIAL.md Outdated Show resolved Hide resolved
JosepBernad and others added 2 commits May 15, 2019 12:01
Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Looks good to me as well, thanks for your first contribution! Let's get it merged. 😉

@PyvesB PyvesB merged commit 17446ef into badges:master May 15, 2019
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Developer and end-user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants