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

popd: add page #1517

Merged
merged 8 commits into from
Oct 11, 2017
Merged

popd: add page #1517

merged 8 commits into from
Oct 11, 2017

Conversation

juan88
Copy link
Contributor

@juan88 juan88 commented Oct 5, 2017


  • The page (if new), does not already exist in the repo.

  • The page (if new), has been added to the correct platform folder:
    common/ if it's common to all platforms, linux/ if it's Linux-specific, and so on.

  • The page has 8 or fewer examples.

  • The PR is appropriately titled:
    <command name>: add page for new pages, or <command name>: <description of changes> for pages being edited

  • The page follows the contributing guidelines

@tldr-bot
Copy link

tldr-bot commented Oct 5, 2017

The build for this PR has failed with the following error(s):

pages/common/popd.md:9: TLDR104 Example descriptions should prefer infinitive tense (e.g. write) over present (e.g. writes) or gerund (e.g. writing)
pages/common/popd.md:13: TLDR104 Example descriptions should prefer infinitive tense (e.g. write) over present (e.g. writes) or gerund (e.g. writing)
pages/common/popd.md:16: TLDR008 File should contain no trailing whitespace

Please fix the error(s) and push again.

@tldr-bot
Copy link

tldr-bot commented Oct 5, 2017

The build for this PR has failed with the following error(s):

pages/common/popd.md:9: TLDR104 Example descriptions should prefer infinitive tense (e.g. write) over present (e.g. writes) or gerund (e.g. writing)
pages/common/popd.md:13: TLDR104 Example descriptions should prefer infinitive tense (e.g. write) over present (e.g. writes) or gerund (e.g. writing)

Please fix the error(s) and push again.

@agnivade
Copy link
Member

agnivade commented Oct 5, 2017

@katekyan / @juan88 - Please remove the commit for the timeout page and do a force push again. Thanks.

@juan88
Copy link
Contributor Author

juan88 commented Oct 5, 2017 via email

@agnivade
Copy link
Member

agnivade commented Oct 5, 2017

So, ideally a single branch should contain commits pertaining to only that PR. In your case, you have the popd branch. This branch has now 4 commits. And we need to get rid of the last commit and push again. The following is what I would do, but there are different ways of achieving this.

In your current branch, do git reset --hard HEAD^. This deletes the last commit. And then do a git push -f origin popd. That should update the remote branch with the new changes. That's it.

For further assistance, @waldyrious might be able to help you out.

@juan88
Copy link
Contributor Author

juan88 commented Oct 5, 2017 via email

@agnivade agnivade added the new command Issues requesting creation of a new page or PRs adding a new page for a command. label Oct 5, 2017
Copy link
Member

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

I think this PR is titled wrong! This looks like a good page for popd, not pushd lol :P

Updating PR title.

@@ -0,0 +1,15 @@
# Popd
Copy link
Member

Choose a reason for hiding this comment

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

Popd should be all lowercase here.


`popd`

- Remove the Nth directory (starting from zero to the left from the list printed with dirs):
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps enclosing dirs in backticks would aid clarity here?


`popd +N`

- Remove the Nth directory (starting from zero to the right from the list printed with dirs):
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as requested.

@sbrl sbrl changed the title pushd: add page popd: add page Oct 6, 2017
@juan88
Copy link
Contributor Author

juan88 commented Oct 6, 2017 via email

@agnivade
Copy link
Member

agnivade commented Oct 6, 2017

@juan88 - Please check for the command in osx and windows platforms. If not, duplicate the page into their respective folders.

@juan88
Copy link
Contributor Author

juan88 commented Oct 6, 2017

@agnivade Moved the page to osx and linux since in windows the command takes less parameters and the examples are more appropiate for osx/linux.

The same happens with pushd. In windows, parameters for the command are different. Should be still placed under common? Want me to open a new issue with this?

https://technet.microsoft.com/en-us/library/bb490969.aspx
https://technet.microsoft.com/en-us/library/bb490978.aspx

@agnivade
Copy link
Member

agnivade commented Oct 7, 2017

Very interesting. In this case, the page should have 3 different copies across the 3 folders. Please add another page under windows, with the windows specific parameters. Thanks for looking into this.

@juan88
Copy link
Contributor Author

juan88 commented Oct 8, 2017

@agnivade

Please add another page under windows, with the windows specific parameters

Do you mean common folder or should I create a new folder pages/windows for this issue? Thanks!

@agnivade
Copy link
Member

agnivade commented Oct 8, 2017

Oh my bad .. I didn't realise that we don't have a windows folder at all. Yes, please create the new folder and add the page there.

@tldr-bot
Copy link

tldr-bot commented Oct 9, 2017

The build for this PR has failed with the following error(s):

pages/windows/popd.md:3: TLDR004 Command descriptions should end in a period
pages/windows/popd.md:1: TLDR009 Page should contain a newline at end of file

Please fix the error(s) and push again.

@@ -0,0 +1,15 @@
# popd

> Remove a directory placed on the directory stack via the pushd shell built-in.
Copy link
Member

Choose a reason for hiding this comment

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

"via the pushd shell built-in" => "by the pushd command"
To make it consistent with the windows page. And also command sounds better than shell built-in. 😆

@agnivade
Copy link
Member

agnivade commented Oct 9, 2017

@juan88 - Thanks for being patient with the review process. We are nearly there. Just one small thing to take care of.

@sbrl
Copy link
Member

sbrl commented Oct 9, 2017

The new windows folder begins! If I end up using a windows machine in the near future and I remember, I'll look at sending a PR for a few windows-specific commands :P

@juan88
Copy link
Contributor Author

juan88 commented Oct 10, 2017

Sure! No problem! Always glad to help and learn! 👍

@sbrl sbrl merged commit 4024334 into tldr-pages:master Oct 11, 2017
@sbrl
Copy link
Member

sbrl commented Oct 11, 2017

Thanks, @juan88! Thanks for sticking with it throughout the review process 😺

@juan88
Copy link
Contributor Author

juan88 commented Oct 11, 2017

No problem! Thanks for your patience! It's nice to be able to contribute to open source projects.

@sbrl sbrl mentioned this pull request Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Issues requesting creation of a new page or PRs adding a new page for a command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants