Skip to content

Updated README.md, added space after -p option #37617

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 31, 2024
Merged

Updated README.md, added space after -p option #37617

merged 3 commits into from
Mar 31, 2024

Conversation

explorest
Copy link
Contributor

@explorest explorest commented Mar 16, 2024 •
edited
Loading

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 16, 2024

please explain in the PR description why this is an improvement

@explorest
Copy link
Contributor Author

explorest commented Mar 16, 2024 •
edited
Loading

Standard syntax of the publishing option for docker run command is to have a space after --publish or -p and before the subsequent ip or port. Docker's official documentation is very clear and consistent on this
Having a space (or an equality sign) between an option's key and its value are universal best practices -- not just on docker's CLI -- but across the industry. It is very unusual , if not outright buggy, to not have a separator ( either a space or an equality) between the key and value on command line.

mkoeppe
mkoeppe approved these changes Mar 16, 2024
View reviewed changes
Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

Thanks.

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 78cf7db; changes) is ready! 🎉

@explorest
Copy link
Contributor Author

Sir @mkoeppe I have no idea what's happening to this pull request. Is it in progress, waiting for something , do I have to do something ?? Sorry to bother you , I am not very fluent with github. Its so confusing.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 24, 2024

No action needed. The label "s: positive review" means that the release manager will be merging it.

@vbraun vbraun merged commit e5c5426 into sagemath:develop Mar 31, 2024
@explorest explorest deleted the patch-1 branch April 1, 2024 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@mkoeppe mkoeppe mkoeppe approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: docker c: documentation
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
3 participants
@explorest @mkoeppe @vbraun