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

Initial implementation of 'new' command #1851

Closed
wants to merge 5 commits into from
Closed

Initial implementation of 'new' command #1851

wants to merge 5 commits into from

Conversation

antsok
Copy link

@antsok antsok commented Mar 12, 2021

Fixes #1750

For initial review.

Unit tests added.

@ghost
Copy link

ghost commented Mar 12, 2021

CLA assistant check
All CLA requirements met.

@alex-frankel alex-frankel changed the title Initial implementation of 'new' command to address #1750 Initial implementation of 'new' command Mar 15, 2021
@codecov-io
Copy link

codecov-io commented Mar 16, 2021

Codecov Report

Merging #1851 (25abf59) into main (58851fc) will decrease coverage by 0.07%.
The diff coverage is 69.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1851      +/-   ##
==========================================
- Coverage   95.11%   95.03%   -0.08%     
==========================================
  Files         371      376       +5     
  Lines       21888    22500     +612     
  Branches       15       15              
==========================================
+ Hits        20818    21383     +565     
- Misses       1070     1117      +47     
Flag Coverage Δ
dotnet 95.46% <69.72%> (-0.09%) ⬇️
typescript 26.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Bicep.Cli/Program.cs 55.97% <0.00%> (-37.71%) ⬇️
...rc/Bicep.Cli/CommandLine/Arguments/NewArguments.cs 95.91% <95.91%> (ø)
src/Bicep.Cli.IntegrationTests/ProgramTests.cs 100.00% <100.00%> (ø)
src/Bicep.Cli.UnitTests/ArgumentParserTests.cs 100.00% <100.00%> (ø)
src/Bicep.Cli/CommandLine/ArgumentParser.cs 100.00% <100.00%> (ø)
src/Bicep.Core/Emit/ScopeHelper.cs 91.32% <0.00%> (-0.28%) ⬇️
src/Bicep.Core.Samples/DataSets.cs 100.00% <0.00%> (ø)
src/Bicep.Core/TypeSystem/NamespaceType.cs 100.00% <0.00%> (ø)
src/Bicep.Core/Semantics/NamespaceSymbol.cs 100.00% <0.00%> (ø)
src/Bicep.Core.UnitTests/Utils/PrintHelper.cs 100.00% <0.00%> (ø)
... and 25 more

@alex-frankel
Copy link
Collaborator

Hey @antsok - thanks for submitting this PR.

We want to have a discussion on our side before we give more feedback to you. Basically, our concern is that this command is making http calls which is a net-new capability for the bicep CLI which we have some reservations about. Wanted to give you this update so you know we are not missing this.

Going forward, if you want to submit any other PRs, we ask that you comment on (or create) a relevant issue to make sure the design of the implementation has been reviewed in some capacity prior to beginning any work (per our contribution guide). I know you submitted an issue, but we would have liked to see an indication that you were going to work on it as well as what the implementation was going to look like. This is so that you don't spend any time on something that may not meet our expectations.

In full transparency, we may not be able to accept this PR, but I hope this does not deter you from submitting other PRs in the future! Let me know if there's anything above I can further clarify.

@antsok
Copy link
Author

antsok commented Mar 17, 2021

Hi @alex-frankel and the team,

Thank you for your constructive feedback. I should have paid more attention to the rules of contributing.

@alex-frankel
Copy link
Collaborator

As mentioned earlier, we are going to close this one mainly due to the introduction of HTTP requests. If we still want a "new" command, let's start with what you currently have for bicep new --help as a way to understand the interface, and we can use that as a jumping off point to decide what the design could/should be.

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.

'bicep new'
3 participants