Skip to content

Move code related to Python version to the py-envs component tree.#12236

Merged
ericsnowcurrently merged 18 commits into
microsoft:masterfrom
ericsnowcurrently:py-envs-version-info
Jun 16, 2020
Merged

Move code related to Python version to the py-envs component tree.#12236
ericsnowcurrently merged 18 commits into
microsoft:masterfrom
ericsnowcurrently:py-envs-version-info

Conversation

@ericsnowcurrently
Copy link
Copy Markdown

This mostly involves moving code, and only minor refactoring (to pull the component code out of the DI-based classes).

  • 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.

@ericsnowcurrently ericsnowcurrently added the no-changelog No news entry required label Jun 9, 2020
@ericsnowcurrently ericsnowcurrently requested review from kimadeline and removed request for karthiknadig June 15, 2020 20:31
Comment thread src/client/pythonEnvironments/exec.ts
Comment thread src/client/pythonEnvironments/exec.ts Outdated
Comment thread src/client/pythonEnvironments/pythonVersion.ts Outdated
Comment thread src/client/pythonEnvironments/info.ts Outdated
Comment thread src/client/pythonEnvironments/pythonVersion.ts Outdated
Comment thread src/test/pythonEnvironments/pythonVersion.unit.test.ts Outdated
Comment thread src/test/pythonEnvironments/pythonVersion.unit.test.ts Outdated
Comment thread src/test/pythonEnvironments/pythonVersion.unit.test.ts Outdated
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Copy Markdown

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

Is GitHub not letting me reply to comments because I'm halfway through a review, or because the comments are outdated 🤔

Anyway:

Regardless, while that airbnb link is a little unclear (I could get rid of the * as part of the line?), the Typescript docs indicate that individual names should be imported unless a bunch of stuff is being imported. I don't mind going with convention even if I don't agree with it. 😄

Yeah that's what I was referring to 👍

In this case it is so it is more clear in the test code what names belong to the system-under-test, as opposed to supporting the tests. I drew inspiration from Graham's talk, where he suggested explicitly identifying the system-under-test in tests.

Oh, that's what sut stood for 🙈 I actually didn't find the namespace clearer, since its meaning wasn't obvious to me. I don't think it brings any extra value when we already have the name of the test file, the test description and the "initialization-test calls-result checking" structure of each test.

Comment thread src/client/pythonEnvironments/exec.ts
@ericsnowcurrently ericsnowcurrently merged commit ab795f5 into microsoft:master Jun 16, 2020
@ericsnowcurrently ericsnowcurrently deleted the py-envs-version-info branch June 16, 2020 22:39
@lock lock Bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants