Skip to content

feat: FastAPI debugger - #14606

Merged
Kim-Adeline Miguel (kimadeline) merged 15 commits into
microsoft:mainfrom
Kludex:feat/fastapi-debugger-implementation
Nov 24, 2020
Merged

feat: FastAPI debugger#14606
Kim-Adeline Miguel (kimadeline) merged 15 commits into
microsoft:mainfrom
Kludex:feat/fastapi-debugger-implementation

Conversation

@Kludex

@KludexMarcelo Trylesinski (Kludex) commented Nov 4, 2020

Copy link
Copy Markdown

For #14247

Only tests are missing.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@ghost

Deleted user (ghost) commented Nov 4, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@KludexMarcelo Trylesinski (Kludex) changed the title WIP: feat: fastapi debuggerWIP: feat: FastAPI debuggerNov 4, 2020
@codecov-io

Codecov (codecov-io) commented Nov 4, 2020

Copy link
Copy Markdown

Codecov Report

Merging #14606 into main will increase coverage by 0.21%.
The diff coverage is 80.48%.

Impacted file tree graph

@@ Coverage Diff @@## main #14606 +/- ##
==========================================
+ Coverage 64.89% 65.10% +0.21% 
==========================================
Files 541 542 +1 Lines 25156 25418 +262 Branches 3545 3593 +48 ==========================================
+ Hits 16324 16548 +224 - Misses 8170 8184 +14 - Partials 662 686 +24 
Impacted FilesCoverage Δ
...tension/configuration/debugConfigurationService.ts78.94% <ø> (ø)
src/client/telemetry/index.ts78.12% <ø> (ø)
...debugger/extension/configuration/resolvers/base.ts90.00% <50.00%> (-1.03%)⬇️
...extension/configuration/providers/fastapiLaunch.ts76.66% <76.66%> (ø)
src/client/common/utils/localize.ts91.15% <100.00%> (+0.15%)⬆️
...tension/configuration/providers/providerFactory.ts100.00% <100.00%> (ø)
...bugger/extension/configuration/resolvers/launch.ts91.01% <100.00%> (+0.10%)⬆️
src/client/debugger/extension/serviceRegistry.ts100.00% <100.00%> (ø)
src/client/debugger/extension/types.ts100.00% <100.00%> (ø)
...rc/client/pythonEnvironments/common/commonUtils.ts85.93% <0.00%> (-14.07%)⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f752706...8c094d6. Read the comment docs.

@brettcannon

Copy link
Copy Markdown
Member

Converted to a draft as this is not ready to be reviewed.

Marcelo Trylesinski (@Kludex) I will leave this here for a week, but if you are not able to get your tests ready by then I will close this PR as we try not to leave non-team member PRs as drafts since they tend to linger that way. And please sign the CLA.

@Kludex

Marcelo Trylesinski (Kludex) commented Nov 4, 2020

Copy link
Copy Markdown
Author

Brett Cannon (@brettcannon) I've signed the CLA, I thought the WIP was enough, thank you for moving to draft.

I've added analogous tests to the ones in Flask, as the debugger is similar, but I'm not satisfied with my work. I've used replace to get the filename without extension (and probably there's a smart way to do it) and there are some tests lacking. If someone can give me some feedback, I'll appreciate.

The functionality per se is working, jfyk. But instead of having main.py as default on inputBox, maybe the current file would be a better option, but none of the other debuggers are doing it, so I was wondering if there's a reason...

Questions related to checkbox:

The wiki is updated with any design decisions/details.
Test plan is updated as appropriate.
Has telemetry for enhancements.

Do I need to do something on those?

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

BugA0 Bugs
VulnerabilityA0 Vulnerabilities (and Security Hotspot0 Security Hotspots to review)
Code SmellA0 Code Smells

No Coverage information No Coverage information
0.6%0.6% Duplication

@brettcannon

Copy link
Copy Markdown
Member

Marcelo Trylesinski (@Kludex) no need to worry about those other checkmarks; that's mostly for the team.

As for the best default, whatever makes sense for FastAPI should be it; don't read too much into how the other configurations do things. 😄

@Kludex

Marcelo Trylesinski (Kludex) commented Nov 11, 2020

Copy link
Copy Markdown
Author

I didn't have the time to add the remaining tests, is it possible to not close at least until the weekend? If a pre-review is possible I'll appreciate as well.

@brettcannon

Copy link
Copy Markdown
Member

Marcelo Trylesinski (@Kludex) yep, as long as you're still working on it we can leave it open her for now and make an exception for you. 😄

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Marcelo Trylesinski (@Kludex)! It's pretty straight-forward, looks good to me so far 🙂

We made some changes to our CI checks, if you could merge/rebase main in your branch that would be great!

instead of having main.py as default on inputBox, maybe the current file would be a better option, but none of the other debuggers are doing it, so I was wondering if there's a reason...

Not that I can remember.

Comment threadbuild/conda-functional-requirements.txt
Comment threadsrc/client/debugger/extension/configuration/providers/fastapiLaunch.ts Outdated
Comment thread.eslintignore Outdated
@Kludex

Marcelo Trylesinski (Kludex) commented Nov 18, 2020

Copy link
Copy Markdown
Author

Thank you for the review. I'm going to apply the necessary changes and rebase tomorrow. :)

@Kludex

Marcelo Trylesinski (Kludex) commented Nov 20, 2020

Copy link
Copy Markdown
Author

Kim-Adeline Miguel (@kimadeline) I've rebased and applied the necessary changes. Is there anything else?

P.S.: I'm unable to see the coverage report... I'm waiting for it to be updated here.

@KludexMarcelo Trylesinski (Kludex) changed the title WIP: feat: FastAPI debuggerfeat: FastAPI debuggerNov 20, 2020
@Kludex
Marcelo Trylesinski (Kludex) marked this pull request as ready for review November 20, 2020 21:18
@Kludex

Copy link
Copy Markdown
Author

When I open the debugger, I have the default being main.py (which is the default name for files that contains a FastAPI application). But maybe having the current open file path would be a better idea. What do you think?

How can I get the tests/test_union_inherited_body.py? (Check the image below)

image

@int19h

Copy link
Copy Markdown

Marcelo Trylesinski (@Kludex) Apologies for slow response times - many people are away on Thanksgiving week.

For the file name, since the configuration is going to persist once it's created, it's probably best to not pick up some transient state such as selected editor tab - if somebody is setting up FastAPI debugging for the first time, they're likely to just click through the wizard on the assumption that the defaults make sense; if it picks the default from tab selection, it's likely to be wrong more often than picking the standard entrypoint. OTOH users who know what they're doing will just enter the correct value there for their custom layout.

@Kludex

Copy link
Copy Markdown
Author

Pavel Minaev (@int19h) Got it! Thank you!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be great if you could address these minor housekeeping items, but other than that great work! Thank you for your contribution Marcelo Trylesinski (@Kludex) 🙂

Comment threadsrc/client/telemetry/index.ts Outdated
@brettcannon

Copy link
Copy Markdown
Member

Marcelo Trylesinski (@Kludex) don't wait for code coverage; it seems to be broken at the moment.

@Kludex

Copy link
Copy Markdown
Author

Kim-Adeline Miguel (@kimadeline) Thank you for the review :) I've applied the necessary changes.

Marcelo Trylesinski (@Kludex) don't wait for code coverage; it seems to be broken at the moment.

Brett Cannon (@brettcannon) Ok, thank you. 👍

@kimadeline
Kim-Adeline Miguel (kimadeline) merged commit f1d0c60 into microsoft:mainNov 24, 2020
@brettcannon

Copy link
Copy Markdown
Member

Thanks for the hard work on this PR, Marcelo Trylesinski (@Kludex) !

@Kludex

Copy link
Copy Markdown
Author

Thank you for all the attention. You guys are great! :) Brett Cannon (@brettcannon)Pavel Minaev (@int19h)Kim-Adeline Miguel (@kimadeline)

Sign up for freeto 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.

5 participants

@Kludex@codecov-io@brettcannon@int19h@kimadeline