-
Notifications
You must be signed in to change notification settings - Fork 0
feat: a bunch of user friendliness changes #4
Conversation
There are a variety of changes here. First, there is a reasonable command line help: ``` ❯ topos-playground -h Usage: topos-playground [options] [command] topos-playground is a CLI tool which handles all of the orchestration necessary to run local Topos devnets with subnets, a TCE network, and apps. Configuration topos-playground follows the XDG Base Directory Specification, which means that data files for use during runs of the playground are store in $XDG_DATA_HOME/topos-playground, which defaults to $HOME/.local/share/topos-playground and log files are stored in $XDG_STATE_HOME/topos-playground/logs, which defaults to $HOME/.local/state/topos-playground/logs. These locations can be overridden by setting the environment variables HOME, XDG_DATA_HOME, and XDG_STATE_HOME. Options: --version Show topos-playground version (v0.0.2) -v, --verbose Show more information about the execution of a command -q, --quiet Show minimal onscreen information about the execution of a command -n, --no-log Do not write a log file -h, --help display help for command Commands: clean Shut down Playground docker containers, and clean up the working directory start Verify that all dependencies are installed, clone any needed repositories, setup the environment, and start all of the docker containers for the Playground version Show topos-playground version (v0.0.2) ``` It intelligently wraps the text to the terminal size. In order for a user to be able to easily know which version of the topos-playground one has installed, topos-playground can be ran with either a `version` command, or a `--version` command line flag. ``` ❯ topos-playground --version topos-playground version 0.0.2 ❯ topos-playground version topos-playground version 0.0.2 ``` The `start` command was modified to give more feedback about what it is doing. The `clean` command was substantially rewritten to give information about what it is doing, and to clean up the working directory. There were significant changes to how the playground reports what it is doing. Instead of _just_ writing to a log file, it now, by default, also outputs to the console. This console output can be turned off with the `--quiet` or `-q` command line flags, and creation of a log file can be turned off with the `--no-log` or `-n` command line flags. ``` topos-playground start --no-log ``` ``` topos-playground clean -q ``` Finally, the command properly honors XDG Base Directory Specification standards. This means that by default, topos-playground follows the XDG Base Directory Specification. Thus data file which the playground creates while running are store in $XDG_DATA_HOME/topos-playground, which defaults to $HOME/.local/share/topos-playground and log files are stored in $XDG_STATE_HOME/topos-playground/logs, which defaults to $HOME/.local/state/topos-playground/logs. This behavior can be changed via environment variables. There is currently no command-line argument support for changing these, though it would not be hard to add. ``` HOME=/tmp topos-playground start ``` ``` XDG_DATA_HOME=/tmp/ topos-playground start ``` It will also read these values from a `.env` file, if one exists. Finally, there has been a small amount added to the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review is purely code reading, I haven't tested the CLI yet
src/ReactiveSpawn.ts
Outdated
@@ -1,6 +1,7 @@ | |||
import { ChildProcess, spawn } from 'child_process' | |||
import { userInfo } from 'os' | |||
import { Observable } from 'rxjs' | |||
import { log, logError } from 'src/loggers' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that I didn't follow this rule consistently myself in the rest of the code (probably due to the initial very temporary characteristic of this tool) but the standard I follow for imports is to separate by an empty line third party imports from locals
import { Observable } from 'rxjs'
import // local
and to use relative paths for local imports
import { log, logError } from './loggers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires have been adjusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would still need to add an empty line to separate local imports from packages (this was my comment above)
src/globals.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the way the logger is used via globals is not really following how nestjs (core library used in this project) works with dependency injection (see here how loggers should be used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original approach wasn't really significantly different, except that it leaked a lot more of the low level logging details into the rest of the code. It just created two Winston loggers, one for the file and one for the console, and then there was code peppered throughout the clean and start commands to directly use those, and everything imported from loggers and assigned the logger object to private variables.
This approach keeps the use of Winston, but hides all of the implementation details behind the log()
and logError()
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about keeping or not Winston (which isn't related to nestjs) but about using globals instead of injecting a custom service/middleware/pipe that would expose the same log
and logError
functions. This is where we're currently deviating from how nestjs wants you to work with it (but we have no time now to refactor this anyway). I'm not sure why we're passing the loggers as global variables.
src/loggers.ts
Outdated
let lines = logMessage.split('\n') | ||
|
||
for (let line of lines) { | ||
if (overrideQuiet || !globalThis.quiet) logConsole().info(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need to be on two different lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistically, or syntactically? It's fine syntactically.
I think the standard JS style is to make it two lines, though a lot of people do write simple if statements in a single line like that. My habit of doing it probably reflects my years of using other languages, and prettier doesn't object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistically yes, I think it's rarely seen in the js community (e.g., forbidden in the airbnb styling which is a reference from what I've seen over the years)
…some notes that I had made while getting this up and running originally. One stumbling block to running it is that if one is _already_ running Redis, one gets the error recorded here. My intent is to add a short troubleshooting section to the README that helps with a few likely common stumbling blocks to using the Playground, including this issue. Removing this file, though. It wasn't intended to be permanent.
… line, and making them all relative paths.
Co-authored-by: Sébastien Dan <[email protected]>
Co-authored-by: Sébastien Dan <[email protected]>
…ore clearly indicate that those are getter methods.
src/commands/start.command.ts
Outdated
return of( | ||
this._spawn.reactify('docker --version'), | ||
defer(() => of(this._log('✅ Docker'))) | ||
new Observable((subscriber) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revert this change as 1. the use of a local variable in between observable isn't super clean 2. this change wasn't replicated to the other dependency installation checks
Resolved some longstanding bugs in the code that does the intelligent line wrapping. Made changes that resolve the excessive wordiness to the console. Fixed some orchestration problems that were resulting in lines being out of order. Added the version checks back, cleaner, and checking both docker and nodejs. There is certainly a minimum version for git, too, but determining that is not straightforward, and unlike the other two dependencies, it is not likely to be an issue.
50bf37d
to
ac4db49
Compare
ac4db49
to
c7bf021
Compare
Description
This PR includes a large set of user-friendliness changes. Whether it is this tool or another tool inspired by lessons learned from this one, I am confident that over the long term we will want a tool that makes it easy for someone to easily start a local environment. With that in mind, I worked on creating the set of features and behaviors that I would want from a tool if I were going to be using it frequently.
Additions and Changes
There are a variety of changes here. First, there is a reasonable command line help:
It intelligently wraps the text to the terminal size. That may or may not show well here in GitHub, but if you install the tool from the branch, and then just do a
topos-playground -h
you will see that it word wraps to maintain readability on any terminal size.In order for a user to be able to easily know which version of the topos-playground one has installed, topos-playground can be ran with either a
version
command, or a--version
command line flag.The version number is taken from the package.json, so as long as this is maintained, it will automatically be picked up. Likewise, the first part of the command description in the help is pulled from the package.json
description
, as well, under the assumption that we want a well written description, and that this description should suffice for the high level introduction to the command in the help, as well.The
start
command was modified to give more feedback about what it is doing.The
clean
command was substantially rewritten to give information about what it is doing, and to clean up the working directory.There were significant changes to how the playground reports what it is doing. Instead of just writing to a log file, it now, by default, also outputs to the console. This console output can be turned off with the
--quiet
or-q
command line flags, and creation of a log file can be turned off with the--no-log
or-n
command line flags.There is a
--verbose
flag that can be used to gain more insight into what is happening. Currently, the only additional information is the commands that are being ran. If there is something that is failing to work correctly, running with--verbose
on can provide more insight into exactly what is happening.Finally, the command properly honors XDG Base Directory Specification standards. This means that by default, topos-playground follows the XDG Base Directory Specification. Thus data file which the playground creates while running are store in $XDG_DATA_HOME/topos-playground, which defaults to $HOME/.local/share/topos-playground and log files are stored in $XDG_STATE_HOME/topos-playground/logs, which defaults to $HOME/.local/state/topos-playground/logs.
This behavior can be changed via environment variables. There is currently no command-line argument support for changing these, though it would not be hard to add.
It will also read these values from a
.env
file, if one exists.Finally, there has been a small amount added to the README.
Breaking changes
The only potential breaking change is the change in the directly locations. However, functionally, this version should be doing the same things to setup or to teardown a local devnet than the version in
main
that it was branched from.PR Checklist:
There are currently no specs in this codebase.