chore: improve code quality and documentation - #1
AungMyoKyaw wants to merge 1 commit into
Conversation
- Add .gitignore for common editor, OS, and temporary files
- Improve README with better structure, examples, and installation methods
- Fix alt text in demo image (was "dotfiles", now "docker-run-zsh demo")
- Add input validation and error handling to all functions
- Use local variables instead of global caps (security improvement)
- Use ${PWD} instead of $(pwd) for better performance
- Fix typo "demon" -> "daemon" in comments
- Add usage helper function for better user experience
- Add comprehensive function documentation
- Add shebang and file header documentation
There was a problem hiding this comment.
Pull request overview
This PR improves the docker-run-zsh plugin by adding comprehensive documentation, input validation, better variable scoping, and a .gitignore file. The changes focus on code quality, security, and user experience improvements.
Changes:
- Added input validation with helpful error messages and usage function
- Converted global uppercase variables to local lowercase variables for better security
- Enhanced documentation with function headers, comprehensive README updates, and fixed typos
- Added
.gitignoreto exclude common editor, OS, and temporary files
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docker-run-zsh.plugin.zsh | Added shebang, header documentation, input validation, usage helper function, and converted to local variables |
| README.md | Enhanced with features list, practical examples, installation methods (Antigen, manual), requirements section, and improved formatting |
| .gitignore | Added patterns for editor/IDE files, OS-generated files, temporary files, and test files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/bash | ||
| else | ||
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/bash |
There was a problem hiding this comment.
The path to bash should be /bin/bash (with a leading slash) instead of bin/bash. Without the leading slash, Docker will look for bin/bash relative to the working directory instead of as an absolute path, which will likely fail.
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/bash | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/bash | |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" /bin/bash | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" /bin/bash |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/bash | ||
| else | ||
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/bash |
There was a problem hiding this comment.
The path to bash should be /bin/bash (with a leading slash) instead of bin/bash. Without the leading slash, Docker will look for bin/bash relative to the working directory instead of as an absolute path, which will likely fail.
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/bash | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/bash | |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" /bin/bash | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" /bin/bash |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/sh | ||
| else | ||
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/sh |
There was a problem hiding this comment.
The path to sh should be /bin/sh (with a leading slash) instead of bin/sh. Without the leading slash, Docker will look for bin/sh relative to the working directory instead of as an absolute path, which will likely fail.
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/sh | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/sh | |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" /bin/sh | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" /bin/sh |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/sh | ||
| else | ||
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/sh |
There was a problem hiding this comment.
The path to sh should be /bin/sh (with a leading slash) instead of bin/sh. Without the leading slash, Docker will look for bin/sh relative to the working directory instead of as an absolute path, which will likely fail.
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" bin/sh | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" bin/sh | |
| docker run --rm -it -v "${PWD}":/src:cached -p "${port}":"${port}" "${image}" /bin/sh | |
| else | |
| docker run --rm -it -v "${PWD}":/src:cached "${image}" /bin/sh |
Summary
This PR improves code quality, security, and documentation for the docker-run-zsh plugin.
Changes
.gitignore- Ignores common editor/IDE files, OS generated files, and temporary files_docker_run_usage()helper functionlocalvariables instead of global uppercase variables (security best practice)${PWD}instead of$(pwd)for better performance and reliabilitySecurity Improvements
IMAGE,PORT) to local lowercase variables (image,port) to prevent variable pollution in the shell environmentTest plan
rdkwithout arguments - should show usage errorrdk ubuntu:latest- should run container with bashrdk node:18 3000- should run container with port mappingrdksh alpine:latest- should run container with shrdkd redis:latest 6379- should run container in detached mode