-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(onboarding): add poetry as an option to python install snippets #90365
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
Changes from all commits
9f7b966
03dc6fd
47d150e
59909c0
8ad4ca7
2ceb0cc
d3dc7e6
05b76d3
701bcf7
2e9a873
3689501
161a5a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,33 @@ | ||||||
// Mock the modules that cause circular dependencies | ||||||
jest.mock('sentry/utils/gettingStartedDocs/python', () => { | ||||||
const original = jest.requireActual('sentry/utils/gettingStartedDocs/python'); | ||||||
return { | ||||||
...original, | ||||||
// Mock any functions causing circular dependencies | ||||||
getPythonProfilingOnboarding: jest.fn(), | ||||||
}; | ||||||
}); | ||||||
|
||||||
import {getPythonInstallSnippet} from 'sentry/utils/gettingStartedDocs/python'; | ||||||
|
||||||
describe('getPythonInstallSnippet', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we maybe write those tests against
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will be resolved in #91119 |
||||||
it('generates install commands with default parameters', () => { | ||||||
const result = getPythonInstallSnippet({ | ||||||
packageName: 'sentry-sdk', | ||||||
}); | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
expect(result.pip).toBe(`pip install "sentry-sdk"`); | ||||||
expect(result.uv).toBe(`uv add "sentry-sdk"`); | ||||||
expect(result.poetry).toBe(`poetry add "sentry-sdk"`); | ||||||
}); | ||||||
|
||||||
it('generates pip install command with minimum version and extras', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
now we test more than just pip :) |
||||||
const result = getPythonInstallSnippet({ | ||||||
packageName: 'sentry-sdk[with-extras]', | ||||||
minimumVersion: '2.3.4', | ||||||
}); | ||||||
expect(result.pip).toBe(`pip install --upgrade "sentry-sdk[with-extras]>=2.3.4"`); | ||||||
expect(result.uv).toBe(`uv add --upgrade "sentry-sdk[with-extras]>=2.3.4"`); | ||||||
expect(result.poetry).toBe(`poetry add "sentry-sdk[with-extras]>=2.3.4"`); | ||||||
}); | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,36 +12,38 @@ import {t, tct} from 'sentry/locale'; | |
|
||
export function getPythonInstallSnippet({ | ||
packageName, | ||
packageManager = 'pip', | ||
minimumVersion, | ||
}: { | ||
packageName: string; | ||
minimumVersion?: string; | ||
packageManager?: 'pip' | 'uv'; | ||
}) { | ||
// We are using consistent double quotes here for all package managers after aligning with the Python SDK team. | ||
// Not using quotes may lead to some shells interpreting the square brackets, and using double quotes over single quotes is a convention. | ||
const versionedPackage = minimumVersion | ||
? `${packageName}>=${minimumVersion}` | ||
: packageName; | ||
? `"${packageName}>=${minimumVersion}"` | ||
: `"${packageName}"`; | ||
|
||
const upgradeFlag = minimumVersion ? '--upgrade ' : ''; | ||
|
||
const packageManagerCommands = { | ||
uv: `uv add ${upgradeFlag}${versionedPackage}`, | ||
pip: `pip install ${upgradeFlag}${versionedPackage}`, | ||
poetry: `poetry add ${versionedPackage}`, | ||
}; | ||
|
||
return packageManagerCommands[packageManager].trim(); | ||
return packageManagerCommands; | ||
} | ||
|
||
export function getPythonInstallConfig({ | ||
packageName = "'sentry-sdk'", | ||
packageName = 'sentry-sdk', | ||
description, | ||
minimumVersion, | ||
}: { | ||
description?: React.ReactNode; | ||
minimumVersion?: string; | ||
packageName?: string; | ||
} = {}): Configuration[] { | ||
const packageManagerCommands = getPythonInstallSnippet({packageName, minimumVersion}); | ||
return [ | ||
{ | ||
description, | ||
|
@@ -51,21 +53,19 @@ export function getPythonInstallConfig({ | |
label: 'pip', | ||
value: 'pip', | ||
language: 'bash', | ||
code: getPythonInstallSnippet({ | ||
packageName, | ||
packageManager: 'pip', | ||
minimumVersion, | ||
}), | ||
code: packageManagerCommands.pip, | ||
}, | ||
{ | ||
label: 'uv', | ||
value: 'uv', | ||
language: 'bash', | ||
code: getPythonInstallSnippet({ | ||
packageName, | ||
packageManager: 'uv', | ||
minimumVersion, | ||
}), | ||
code: packageManagerCommands.uv, | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since all of the 3x packages are being used here. what about just return
in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, changed it 👍 |
||
label: 'poetry', | ||
value: 'poetry', | ||
language: 'bash', | ||
code: packageManagerCommands.poetry, | ||
}, | ||
], | ||
}, | ||
|
@@ -93,7 +93,7 @@ export function getPythonAiocontextvarsConfig({ | |
); | ||
|
||
return getPythonInstallConfig({ | ||
packageName: "'aiocontextvars'", | ||
packageName: 'aiocontextvars', | ||
description: description ?? defaultDescription, | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need it, because otherwise the tests don't work