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

Memory viz cli #43

Merged
merged 15 commits into from
Jun 19, 2024
Merged

Conversation

sarahsonder
Copy link
Collaborator

@sarahsonder sarahsonder commented Jun 10, 2024

Proposed Changes

The purpose of this pull request is to create a command line interface for MemoryViz. This is to make it easier to run MemoryViz.

To run the CLI, run npm i to install the CLI and then run memory-viz <file-to-path>, replacing <path-to-file> with the path to a file containing MemoryViz-compatible JSON

Type of Change

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
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

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

  1. I am unsure as the why the Github tests are failing. The issue seems to be that the files/branch cannot be found. I left a more detailed message on Slack. Thank you!
  2. Since my current implementation takes in only one argument, which is the path to the file containing the Memory-Viz compatible JSON input, users do not have a choice as to where the SVG image gets saved. This also means that users cannot choose the name of the output, which means that old SVGs could get overwritten. Would you like me to change the implementation such that users may specify where they would like the image to be saved? For reference, I currently have it so that the image gets saved in the same directory in which Memory-Viz is being called. I can also look into renaming files with (1) or (2) if files of the same name already exist in a directory.
  3. Would you like me to create a new page in the docs website to document the CLI or should I do it elsewhere?
  4. Is there a particular library that you would like me to use for testing the CLI?
  5. Are we only allowing JSON files as inputs for the moment?

@sarahsonder sarahsonder requested a review from david-yz-liu June 10, 2024 23:28
@david-yz-liu
Copy link
Owner

@sarahsonder good questions.

  1. For the output file: the current directory is fine. For the filename, take the base name of the input json and change the extension to .svg. (Future work could involve allowing the user to specify an output file.)
  2. For documentation: (1) add a brief subsection to the memory-viz/README.md with a sample invocation, and (2) yes create a new documentation page for the CLI. It can be pretty bare-bones for now, but hopefully it will grow over time as we add new options. 😄 You can use some professional pages as templates to follow, e.g. https://jestjs.io/docs/cli.
  3. You should stick with Jest testing. For now you can manually create a subprocess to execute an npx memory-viz <whatever> and check the file contents (ideally, against a snapshot!).
  4. Yes that's correct, though I'm not sure what other file types you're thinking of.

@sarahsonder sarahsonder marked this pull request as ready for review June 13, 2024 22:20
@sarahsonder

This comment was marked as outdated.

@david-yz-liu
Copy link
Owner

@sarahsonder don't put the pull request template in a comment. Instead edit the pull request description so that it contains the full information.

@sarahsonder sarahsonder marked this pull request as draft June 15, 2024 22:12
@sarahsonder sarahsonder marked this pull request as ready for review June 17, 2024 12:53
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 good work. Regarding the failing tests, there's an error in how the module is being imported (and likely your local environment is different from GitHub Actions).

Try using a different approach:

  1. Modify the GitHub Actions workflow to first run the webpack build command before running the tests
  2. (perhaps try this, not sure) In the test cases, invoke just memory-viz rather than npx memory-viz

CHANGELOG.md Outdated Show resolved Hide resolved
README.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/draw.spec.tsx Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9558806459

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 87.538%

Totals Coverage Status
Change from base Build 9558270699: 0.0%
Covered Lines: 388
Relevant Lines: 429

💛 - Coveralls

@sarahsonder sarahsonder requested a review from david-yz-liu June 18, 2024 04:17
package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
docs/docs/06-cli.md Outdated Show resolved Hide resolved
memory-viz/bin/cli.js Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9572882702

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 87.538%

Totals Coverage Status
Change from base Build 9558270699: 0.0%
Covered Lines: 388
Relevant Lines: 429

💛 - Coveralls

@sarahsonder sarahsonder requested a review from david-yz-liu June 18, 2024 22:12
@david-yz-liu david-yz-liu merged commit da9c3d5 into david-yz-liu:master Jun 19, 2024
3 checks passed
@sarahsonder sarahsonder deleted the memory-viz-cli branch June 19, 2024 19:34
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