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

feat(wokwi): Add support for specifying diagram path #296

Merged
merged 1 commit into from
May 31, 2024

Conversation

P-R-O-C-H-Y
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y commented May 29, 2024

This PR adds a support of specifying diagram file for pytest-embedded-wokwi.
To specify diagram.json add --wokwi-diagram to the pytest command

@hfudev
Copy link
Member

hfudev commented May 30, 2024

Hi @P-R-O-C-H-Y!

The code changes LGTM. However, if I've downloaded the old wokwi-cli release and run the test, the new cli args --wokwi-diagram will cause an exception

.2024-05-30 10:26:43 _ArgError: unknown or unexpected option: --diagram-file
2024-05-30 10:26:43     at arg2 (/snapshot/dist/cli.cjs:126:23)
2024-05-30 10:26:43     at main (/snapshot/dist/cli.cjs:14099:39)
2024-05-30 10:26:43     at Object.<anonymous> (/snapshot/dist/cli.cjs:14317:1)
2024-05-30 10:26:43     at Module._compile (pkg/prelude/bootstrap.js:1926:22)
2024-05-30 10:26:43     at Module._extensions..js (node:internal/modules/cjs/loader:1166:10)
2024-05-30 10:26:43     at Module.load (node:internal/modules/cjs/loader:988:32)
2024-05-30 10:26:43     at Module._load (node:internal/modules/cjs/loader:834:12)
2024-05-30 10:26:43     at Function.runMain (pkg/prelude/bootstrap.js:1979:12)
2024-05-30 10:26:43     at node:internal/main/run_main_module:17:47 {
2024-05-30 10:26:43   code: 'ARG_UNKNOWN_OPTION'
2024-05-30 10:26:43 }

Could you add more code to handle this use case? I can come up with a few solutions here.

  • write backward compatible code (which is quite uncessary here, since wokwi-cli is still in dev-phase)
  • add one global WOKWI_MINIMUM_VERSION, and ask the user to update the wokwi cli version if the version is lower than this value.
  • check and download the latest version of wokwi-cli while setup automatically (cause more network traffic to users, and maybe they're using hotspot or something)

Overall I'll pick solution 2, and welcome any other ideas!

@P-R-O-C-H-Y P-R-O-C-H-Y force-pushed the feat/wokwi-diagram-path-support branch from 7e6eb46 to 66536c2 Compare May 30, 2024 13:14
This PR adds a support of specifying diagram file for pytest-embedded-wokwi.
To specify diagram.json add --wokwi-diagram to the pytest command

Co-Authored-By: Fu Hanxi <hfudev@gmail.com>
@P-R-O-C-H-Y P-R-O-C-H-Y force-pushed the feat/wokwi-diagram-path-support branch from 7894374 to 1383c2e Compare May 31, 2024 09:28
@P-R-O-C-H-Y
Copy link
Member Author

@hfudev All required changes are done :)

@P-R-O-C-H-Y P-R-O-C-H-Y requested a review from hfudev May 31, 2024 09:30
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your PR!

@hfudev hfudev merged commit 9610a21 into espressif:main May 31, 2024
5 checks passed
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