-
-
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
feat(onboarding): add poetry as an option to python install snippets #90365
Conversation
…orgs that don't have dynamic sampling or custom sample rates
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #90365 +/- ##
==========================================
- Coverage 87.82% 83.15% -4.67%
==========================================
Files 10267 10280 +13
Lines 579568 579158 -410
Branches 22622 22648 +26
==========================================
- Hits 508984 481599 -27385
- Misses 70150 97121 +26971
- Partials 434 438 +4 |
we probably want to remove the quotes here too:
|
@@ -67,6 +70,16 @@ export function getPythonInstallConfig({ | |||
minimumVersion, | |||
}), | |||
}, | |||
{ |
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.
since all of the 3x packages are being used here. what about just return
return {
uv: `uv add ${upgradeFlag}${versionedPackage}`,
pip: `pip install ${upgradeFlag}${versionedPackage}`,
poetry: `poetry add ${versionedPackage}`,
}
in the getPythonInstallSnippet
? this way we don't need to call the function 3x times but only 1x and we can do pythonInstallSnippet.poetry
for example
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.
Good point, changed it 👍
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.
From my limited frontend knowledge this looks good to me.
const result = getPythonInstallSnippet({ | ||
packageName: 'sentry-sdk', | ||
}); |
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.
nit:
const result = getPythonInstallSnippet({ | |
packageName: 'sentry-sdk', | |
}); | |
const result = getPythonInstallSnippet(); |
Isn't "sentry-sdk" the default packageName?
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.
Not in the InstallSnippet; only in the InstallConfig, which is why it needs to be passed in here in the test
it('generates uv install command with default parameters', () => { | ||
const result = getPythonInstallSnippet({ | ||
packageName: 'sentry-sdk', | ||
}); | ||
|
||
expect(result.uv).toBe(`uv add "sentry-sdk"`); | ||
}); |
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 could combine the tests for uv, pip, and poetry into a single one - this test , for example, could be combined with the first one
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.
Good point - changed
// 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(), | ||
}; | ||
}); |
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
what about falcon?
|
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.
left a few suggestions, but overall it looks good to me! 🙌
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 comment
The reason will be displayed to describe this comment to others. Learn more.
it('generates pip install command with minimum version and extras', () => { | |
it('generates install commands with minimum version and extras', () => { |
now we test more than just pip :)
const result = getPythonInstallSnippet({ | ||
packageName: 'sentry-sdk', | ||
}); | ||
|
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.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
||
import {getPythonInstallSnippet} from 'sentry/utils/gettingStartedDocs/python'; | ||
|
||
describe('getPythonInstallSnippet', () => { |
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.
can we maybe write those tests against getPythonInstallConfig
instead of getPythonInstallSnippet
?
getPythonInstallSnippet
is only used inside getPythonInstallConfig
, so it’s an implementation detail of getPythonInstallSnippet
. The “public interface” that is used by consumers is getPythonInstallConfig
, and we should be free to change how that is implemented internally without having to change tests
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.
Will be resolved in #91119
Example:
