Skip to content

Append --allow-prereleases to black installation command when Poetry is used #5967

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

Merged
merged 3 commits into from
Jun 20, 2019

Conversation

notpushkin
Copy link

@notpushkin notpushkin commented Jun 11, 2019

Similar to #5393.

For #5756 (it has been mistakenly closed as a duplicate of #5171, but the latter was about Pipenv and the former is about Poetry :)

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

@msftclas
Copy link

msftclas commented Jun 11, 2019

CLA assistant check
All CLA requirements met.

@notpushkin
Copy link
Author

Could you take a look @DonJayamanne? :)

@kimadeline
Copy link

Hi @notpushkin , thank you for submitting this pull request! Could you add some tests for it? Rest looks good 👍

Copy link

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

Add tests (see comment #5967 (comment))

@notpushkin
Copy link
Author

Thanks for a review @kimadeline! I took a look at the existing tests and I see that tests for Poetry are in a separate file and do not iterate over the Product enum (like the tests for pip / pipenv / conda), so I'm not sure where I can plug the test for the changes I've made. Could you give me a hint? :-)

@kimadeline
Copy link

You were on the right track when you mentioned poetryInstaller.unit.test.ts. The last unit test of the suite on line 114 checks the content of the execution info array, so you could append a new test that asserts equality when the module name is black:

test('Get executable info when installing black', async () => {
        const uri = Uri.file(__dirname);
        const settings = mock(PythonSettings);

        when(configurationService.getSettings(uri)).thenReturn(instance(settings));
        when(settings.poetryPath).thenReturn('poetry path');

        const info = await poetryInstaller.getExecutionInfo('black', uri);

        assert.deepEqual(info, { args: ['add', '--dev', 'black', '--allow-prereleases'], execPath: 'poetry path' });
    });

@DonJayamanne DonJayamanne removed this from the 2019 - June Sprint 12 milestone Jun 19, 2019
@notpushkin
Copy link
Author

Thank you so much @kimadeline! Added the test case and it works alright:

$ npm run test:unittests -- --grep='installing black'

> [email protected] test:unittests /home/ale/dev/vscode-python
> mocha --require source-map-support/register --opts ./build/.mocha.unittests.opts "--grep=installing black"

(node:1381) [DEP0016] DeprecationWarning: 'root' is deprecated, use 'global'
  Module Installer - Poetry
    ✓ Get executable info when installing black

  1 passing 

@kimadeline kimadeline merged commit 16899f6 into microsoft:master Jun 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants