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

Make CLI Commands more independent #34

Merged
merged 8 commits into from Jan 30, 2021

Conversation

JordanShurmer
Copy link
Contributor

Note: this PR is to the cli-refactor branch, not master.

What Changed

  • The commands in cli/ are more independent and self-actualizing
    • They export their own usage string
    • They parse their own command line options
  • cli.js is cleaned up
    • it only parses the options that it cares about
    • handles printing usage info for every command
    • exports the current version

I Need Help

I'm not sure how to implement the run command. This PR adds it as an unimplemented() piece of code.
We could merge this into the cli-refactor branch then fix the run command in there, or we can update this PR if someone can assist in implementing it.

Jordan Shurmer and others added 4 commits January 28, 2021 08:09
- each command exports its own usage info
- each command parses its own cli options
- cli.js handles the --help and --version requests
@JordanShurmer
Copy link
Contributor Author

Test it out here:

deno run --unstable -A https://raw.githubusercontent.com/JordanShurmer/lume/1872c101bd57a943908a5666daa6339da410cdb1/cli.js --help


deno run --unstable -A https://raw.githubusercontent.com/JordanShurmer/lume/1872c101bd57a943908a5666daa6339da410cdb1/cli.js build --help

# etc.

@oscarotero
Copy link
Member

Hi, thanks for your work here.
Both build and run need an instance of site. This is why in my implementation I've created the site before executing these two commands (See here).

Maybe cli.js can export a createSite(options) function that can be imported and used by those two commands (like you did with version).

Then, run only need to execute site.run() with the name of the script. For example: lume run deploy should execute site.run("deploy").

Another thing: can you run deno fmt on finish this pull request? I want to use the same code format of Deno (the only exception is CHANGELOG.md) that should be ignored.

Thanks!

cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
Jordan Shurmer added 3 commits January 29, 2021 21:23
* handle HELP message without a dynamic import
* clean up repetitive code by using an inline function
* create cliUtils file to hold shared functions
@JordanShurmer
Copy link
Contributor Author

Thanks. all, for reviewing.

Made various changes to address the feedback:

  • Implemented the run command
    • pulled the site building logic into a shared function in cliUtils.js
  • Refactored the way cli.js pulls in the commands to avoid doing dynamic import
  • Tried to clean up cli.js even more, let me know what you think
  • deno fmt

@JordanShurmer
Copy link
Contributor Author

JordanShurmer commented Jan 30, 2021

Test out the latest here: https://raw.githubusercontent.com/JordanShurmer/lume/39514dad3dd2f5f5ce92c8aabbbf7eb53735cd8b/cli.js

deno run --unstable -A https://raw.githubusercontent.com/JordanShurmer/lume/39514dad3dd2f5f5ce92c8aabbbf7eb53735cd8b/cli.js --help

@oscarotero oscarotero merged commit 81854ce into lumeland:feature/cli-refactor Jan 30, 2021
@oscarotero
Copy link
Member

Great job. Thank you @JordanShurmer

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.

None yet

3 participants