fix: update paragon build command options #37
Conversation
- Add --exclude-core option to the build command - Change token references option to --output-token-references/--no-output-token-references - Update help text and argument handling accordingly
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Ang-m4! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I left one comment with a specific question, but seeing this does make me wonder if manually wrapping the Paragon CLI this much makes sense.
With this pattern every time the Paragon CLI is updated to add a new option this plugin will need to be updated to wrap that option. That doesn't feel ideal to me.
Would it be possible to simplify the wrapper to just pass the Paragon CLI args through unmodified?
|
@brian-smith-tcril, thanks for your comments! I’ve simplified the paragon-builder command as you suggested, so it now forwards all arguments and options unchanged. This also means our tests in #36 no longer need to validate each flag or argument that belongs in Paragon’s own test suite. Let me know what you think! |
That's wonderful! Thank you!
I think the spirit of integration testing would imply we should still test arguments we expect users to use. We already have tests on the Paragon side for this, but verifying that nothing breaks when running the commands through the plugin still feels like a good thing for the plugin to have tests for. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
This is great! I'm really happy to see how much simpler this can be when just forwarding the args.
I left a comment about the docs, once that's addressed this should be good to merge!
|
|
||
| - ``--output-token-references`` | ||
| Include references for tokens with aliases to other tokens in the build output. | ||
| - ``--output-token-references/--no-output-token-references`` |
There was a problem hiding this comment.
Instead of having a full list of available options that get passed through to the paragon CLI it might make sense to just link to the Paragon CLI docs (https://github.com/openedx/paragon/?tab=readme-ov-file#paragon-cli). If there's something you feel should be documented that isn't documented on the Paragon repo I'll gladly approve a PR adding it!
Ang-m4
force-pushed
the
afg/fix-paragon-flags
branch
from
470536b to
9d61529
Compare
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
This is perfect! Thank you so much for working through the review process with me!
3 participants
Description
This PR corrects the behavior of the
--output-token-referencesoption so it properly toggles token-reference output, and adds support for the existing--exclude-corefeature in the Python CLI binding.Key Changes
Fix
--output-token-references--output-token-references/--no-output-token-references), with references enabled by default.Add support for
--exclude-core--exclude-coreflag to theparagon-build-tokenscommand, giving users the option to omit the core token set from the build output.All related function docstrings, CLI help text, and README examples have been updated to reflect these changes.