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

Add necessary commands for VSCode Devcontainers #394

Merged
merged 5 commits into from
Dec 30, 2021
Merged

Add necessary commands for VSCode Devcontainers #394

merged 5 commits into from
Dec 30, 2021

Conversation

howlowck
Copy link
Contributor

Changes:

  • Added short and format args to version command
  • Added a trivial implementation of the config command which echos out the content when given an absolute path of a file

Resolves:

@howlowck howlowck changed the title Added necessary commands for VSCode Devcontainers Add necessary commands for VSCode Devcontainers Dec 30, 2021
@muayyad-alsadi
Copy link
Collaborator

does vscode expects
version on stdout or stderr?

the following line should be replaced with log(...) (it just a helper that just print to stderr)

        sys.stderr("File path is required. use -f flag to set the input file")
...
    if (file_path == None):
        sys.stderr("File path is required. use -f flag to set the input file")
        return

even more, this should be part of parsing, which is handled by argparse not need at all to process it

parser.add_argument("-f", "--format", type=str, default='pretty', 
        help="Format the output. Values: [pretty | json].")

should have choices like this

parser.add_argument("-f", "--format", choices=['pretty', 'json'], default='pretty'
                            help="Format the output")

please don't construct json by substitution and concatenation and replace, use json.dumps

        print('{ "version": "{version}" }'.replace('{version}', __version__))

should be

        res = {"version": __version__}
        print(json.dumps(res))

@howlowck
Copy link
Contributor Author

Hey @muayyad-alsadi, thanks for the quick review. vscode expects stdout. I will make the suggested updates.

@howlowck
Copy link
Contributor Author

Updated. Thanks again!

@muayyad-alsadi muayyad-alsadi merged commit 31df70b into containers:devel Dec 30, 2021
@muayyad-alsadi
Copy link
Collaborator

I'll do some changes and need your help testing them.

muayyad-alsadi added a commit that referenced this pull request Dec 30, 2021
@muayyad-alsadi
Copy link
Collaborator

I've made config prints the merged yaml of multiple files
please test

@howlowck
Copy link
Contributor Author

Yes. Multiple files are merging properly.

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.

2 participants