Conversation
🦋 Changeset detectedLatest commit: 9dc1805 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
CodSpeed Performance ReportMerging #14609 will not alter performanceComparing Summary
Footnotes |
github-actions
bot
added
the
pkg: integration
florian-lefebvre
added
the
pr preview
github-actions
bot
removed
the
pr preview
commit: |
|
Everything seems to work fine on my side (EndeavourOS with XFCE), nice job! Did you want us to test specific scenarios? I only checked using a minimal project. |
|
No it's fine thanks! |
| @@ -0,0 +1 @@ | |||
| export type DebugInfo = Array<[string, string | Array<string>]>; | |||
There was a problem hiding this comment.
Should this go to info/definitions.ts?
There was a problem hiding this comment.
I don't think so. Currently the structure I've been following is:
- domain/filename.ts: a specific data entity
- definitions.ts: infrastructure definitions
- infra/filename.ts: infrastructure implementation
- core/filename.ts: one function holding some core logic
There was a problem hiding this comment.
This isn't documented anywhere:
- PR
- Contribution guidelines
- comments
You raised a similar PR for fonts now, and I would expect the same infrastructure. Should address this comment first?
i.e. you explained to me the folder structure, but I still don't understand why debug-info.ts must be here... honestly, it's all foggy. Maybe let's raise a PR that explains this folder/business logic structure.
Not a blocker, but something you should consider
There was a problem hiding this comment.
Good point, I'll make a follow up PR with guidelines
packages/astro/src/cli/info/infra/process-package-manager-user-agent-provider.ts
Show resolved
Hide resolved
|
|
||
| export function createNpmPackageManager({ commandExecutor }: Options): PackageManager { | ||
| return { | ||
| getName() { |
There was a problem hiding this comment.
nit (to apply to all interfaces)
| getName() { | |
| get name() { |
There was a problem hiding this comment.
Noted! I will do this in a follow up PR to all relevant code. I don't want to do it in this PR to avoid making the diff bigger
|
@ematipico thanks for the reviews! I addressed everything, PTAL |
3 participants
Changes
astro infoCLI commandastro infois retrieved in dev for the dev toolbarTesting
Docs