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

Adding CLI Streams #52

Merged
merged 14 commits into from
Jul 19, 2024
Merged

Conversation

sarahsonder
Copy link
Collaborator

@sarahsonder sarahsonder commented Jul 9, 2024

Proposed Changes

This PR allows for new input and output streams to the CLI. Specifically, the <file-to-path> argument was made optional, allowing users to also use stdin as input if a file path is not provided. Users can also redirect output to a location of their choice using --output<path> or print the SVG directly to standard output.

Type of Change

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change) X
New feature (non-breaking change that adds functionality) X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported by Coveralls.
  • I have requested a review from a project maintainer.

Questions and Comments

Hi Professor Liu! So far, I have made the --filepath argument optional and I have also added the options --stdout, --output, and --stdin. However, I have a few questions about the --stdin option.

  1. As a general sanity check, are my current changes in the right direction?
  2. When using standard input, how exactly do you want users to give input? For example, in the terminal, would you want users to enter npx memory-viz --stdin, press enter, type in the json, and then press enter again to run memory-viz? I'm still working on getting input directly from the terminal, but I can currently get the command Write-Output '[{ "type": "int", "id": 13, "value": 7}]'| npx memory-viz --stdin --stdout to work. For reference, I'm using Windows, hence the Write-Output command.
  3. For --stdin, what would you like the output file to be named? I currently have it named as memory_viz.svg
  4. In your slack message, you mentioned that if --stdin is chosen, there's won't be a default path. However, I believe that the default path is wherever memory-viz is being called because Write-Output '[{ "type": "int", "id": 13, "value": 7}]'| npx memory-viz --stdin outputs into my memory-viz folder. That being said, I could be mistaken and this might be different be for users.

Thank you so much!

@sarahsonder sarahsonder requested a review from david-yz-liu July 9, 2024 01:19
@david-yz-liu
Copy link
Owner

@sarahsonder good progress.

  1. Yes you're on the right track.
  2. "reading from stdin" means "read fully from stdin". You learned about various approaches for this in CSC209, and you're correct that one approach is to use a pipe. I think your main confusion lies in that you aren't aware that when just typing in input, the user can use a keyboard shortcut to signal the end of their input. This is likely Ctrl+D for you. In other words, the "Enter" key should not be treated in any special way; instead, the user should be able to type in as much JSON as they like, and then signal the end of input with a keyboard shortcut.
    3 and 4. The term "path" includes both directory and filename. Please re-read my instructions with this in mind.

@sarahsonder sarahsonder marked this pull request as ready for review July 12, 2024 07:22
@coveralls
Copy link
Collaborator

coveralls commented Jul 12, 2024

Pull Request Test Coverage Report for Build 10002435167

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.925%

Totals Coverage Status
Change from base Build 9855023876: 0.0%
Covered Lines: 407
Relevant Lines: 438

💛 - Coveralls

@sarahsonder sarahsonder requested review from david-yz-liu and removed request for david-yz-liu July 16, 2024 22:00
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
memory-viz/bin/cli.js Outdated Show resolved Hide resolved
memory-viz/bin/cli.js Outdated Show resolved Hide resolved
memory-viz/bin/cli.js Outdated Show resolved Hide resolved
memory-viz/bin/cli.js Outdated Show resolved Hide resolved
memory-viz/src/tests/cli.spec.tsx Outdated Show resolved Hide resolved
docs/docs/06-cli.md Outdated Show resolved Hide resolved
docs/docs/06-cli.md Outdated Show resolved Hide resolved
docs/docs/06-cli.md Outdated Show resolved Hide resolved
@sarahsonder sarahsonder requested a review from david-yz-liu July 18, 2024 01:39
Copy link
Owner

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

@sarahsonder great progress. I left one inline comment.

Regarding the issue of reading from stdin (on Windows), I looked into this a bit more. Definitely seems like Windows is a culprit here. For your implementation use streamConsumers.json to streamline the process. When I tested it, the function worked when piping but I also was unable to signal an EOF when typing into the terminal.

We can accept that as a possible limitation for now, and please document this limitation.

docs/docs/06-cli.md Outdated Show resolved Hide resolved
@sarahsonder sarahsonder requested a review from david-yz-liu July 19, 2024 04:12
@david-yz-liu david-yz-liu merged commit 058023f into david-yz-liu:master Jul 19, 2024
3 checks passed
@sarahsonder sarahsonder deleted the cli-streams branch July 19, 2024 13:05
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.

3 participants