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

add noProject flag to bsc so BSConfig.json not expected #868

Merged
merged 7 commits into from
Sep 18, 2023
Merged

add noProject flag to bsc so BSConfig.json not expected #868

merged 7 commits into from
Sep 18, 2023

Conversation

philanderson888
Copy link
Contributor

No description provided.

@philanderson888 philanderson888 changed the title initial commit add noProject flag to bsc to BSConfig.json not expected Aug 3, 2023
@philanderson888 philanderson888 changed the title add noProject flag to bsc to BSConfig.json not expected add noProject flag to bsc so BSConfig.json not expected Aug 3, 2023
Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Hey, here are a few suggestions.

If --noProject is present at all, we should disable all project loading, even if the user has supplied --project. That should simplify the logic a bit, and makes logical sense to me

src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/BsConfig.ts Outdated Show resolved Hide resolved
src/util.ts Show resolved Hide resolved
src/ProgramBuilder.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
@philanderson888
Copy link
Contributor Author

Thank you @TwitchBronBron for that clarification - yes I was not sure which way to go, so went for the softer option!

Happy to go with the hard stop so if the flag is set, bsconfig.json loading is completely disabled.

Happy for you to review the code a second time

Thanks

All suggestions implemented, just a few changes to comments but other than that as-is as you specified.

Please check all code and let me know any faults or amendments, thanks

:)

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for this!

@TwitchBronBron TwitchBronBron merged commit 5f2c332 into rokucommunity:master Sep 18, 2023
5 checks passed
@philanderson888
Copy link
Contributor Author

Thanks @TwitchBronBron happy to help

Onwards and upwards to the next one!

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