feat(discovery): add project discovery package#262
feat(discovery): add project discovery package#262pjcdawkins wants to merge 2 commits intorefactor/abstract-fs-2-fsysfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new discovery/ package to centralize project detection logic (stack/runtime/dependency managers/build steps/etc.) over an fs.FS, and wires stack discovery into the existing interactive questionnaire flow.
Changes:
- Add
discovery.Discovererwith memoized detection methods and accompanying unit tests. - Refactor stack detection in
internal/question/stack.goto delegate toDiscoverer.Stack(). - Extend platform stack constants and add a
CountFilesutility to support runtime/type heuristics.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| platformifier/models.go | Adds new stack constants (Symfony, Ibexa, Shopware) and Stack.Name() mappings. |
| internal/utils/utils.go | Adds CountFiles(fs.FS) used for runtime/type heuristics. |
| internal/question/working_directory.go | Initializes answers.Discoverer based on the chosen working directory. |
| internal/question/stack.go | Uses Discoverer.Stack() instead of inline filesystem heuristics. |
| internal/question/stack_test.go | Adds tests verifying stack question behavior with fstest.MapFS. |
| internal/question/models/answer.go | Adds Discoverer to Answers. |
| internal/question/build_steps.go | Restores manage.py constant at package scope after stack refactor. |
| discovery/* | New package implementing discovery logic (stack/type/env/build steps/app root/dependency managers) with tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if keywordsVal, ok := keywords.([]string); ok && slices.Contains(keywordsVal, "shopware") { | ||
| return platformifier.Shopware, nil |
There was a problem hiding this comment.
GetJSONValue() unmarshals JSON arrays as []any, so asserting keywords.([]string) will never succeed and this Shopware detection path is effectively dead code. Consider handling []any and checking each element for the "shopware" string (or adjust GetJSONValue to decode arrays of strings when appropriate).
| if keywordsVal, ok := keywords.([]string); ok && slices.Contains(keywordsVal, "shopware") { | |
| return platformifier.Shopware, nil | |
| // Handle both []string and []any returned by GetJSONValue for JSON arrays. | |
| if keywordsVal, ok := keywords.([]string); ok { | |
| if slices.Contains(keywordsVal, "shopware") { | |
| return platformifier.Shopware, nil | |
| } | |
| } else if keywordsSlice, ok := keywords.([]any); ok { | |
| for _, kw := range keywordsSlice { | |
| if kwStr, ok := kw.(string); ok && kwStr == "shopware" { | |
| return platformifier.Shopware, nil | |
| } | |
| } |
| "yarn/yarn.lock": &fstest.MapFile{}, | ||
| "poetry/poetry.lock": &fstest.MapFile{}, | ||
| "composer/composer-lock.json": &fstest.MapFile{}, |
There was a problem hiding this comment.
Test fixture uses composer/composer-lock.json, but the code detects Composer via composer.lock. As written, this test doesn’t exercise Composer detection at all. Rename the fixture to composer/composer.lock (or adjust the detector if the intended file is different).
| "yarn/yarn.lock": &fstest.MapFile{}, | |
| "poetry/poetry.lock": &fstest.MapFile{}, | |
| "composer/composer-lock.json": &fstest.MapFile{}, | |
| "yarn/yarn.lock": &fstest.MapFile{}, | |
| "poetry/poetry.lock": &fstest.MapFile{}, | |
| "composer/composer.lock": &fstest.MapFile{}, |
| { | ||
| name: "Pipenv Django with Yarn build", | ||
| fields: fields{ | ||
| fileSystem: fstest.MapFS{ | ||
| "project/manage.py": &fstest.MapFile{}, | ||
| "package.json": &fstest.MapFile{Data: []byte(`{"scripts": {"build": "nuxt build"}}`)}, | ||
| }, | ||
| memory: map[string]any{ | ||
| "stack": platformifier.Django, | ||
| "type": "python", | ||
| "dependency_managers": []string{"poetry", "yarn"}, | ||
| "application_root": ".", | ||
| }, |
There was a problem hiding this comment.
The test case name says "Pipenv Django with Yarn build", but the setup uses dependency_managers: []string{"poetry", "yarn"} and asserts Poetry commands. Rename the case (or change the fixture) so the test name matches what it actually covers.
| @@ -51,166 +40,69 @@ func (q *Stack) Ask(ctx context.Context) error { | |||
| }() | |||
|
|
|||
| answers.Stack = models.GenericStack | |||
There was a problem hiding this comment.
answers.Discoverer is dereferenced without a nil check. If Stack.Ask is ever called without WorkingDirectory.Ask having initialized Discoverer, this will panic. Consider lazily initializing it from answers.WorkingDirectory when nil (or returning a clear error) to make the question handler more robust.
| answers.Stack = models.GenericStack | |
| answers.Stack = models.GenericStack | |
| if answers.Discoverer == nil { | |
| return fmt.Errorf("stack discoverer is not initialized") | |
| } |
| HasGit bool `json:"has_git"` | ||
| FilesCreated []string `json:"files_created"` | ||
| Locations map[string]map[string]interface{} `json:"locations"` | ||
| Discoverer *discovery.Discoverer |
There was a problem hiding this comment.
Discoverer is a runtime helper and likely shouldn’t be serialized with Answers (it has no json tag, unlike most fields). Consider adding json:"-" to prevent accidental inclusion in any JSON output/telemetry and to clarify it’s not user-provided data.
| Discoverer *discovery.Discoverer | |
| Discoverer *discovery.Discoverer `json:"-"` |
| func (d *Discoverer) nodeExecPrefix() string { | ||
| dependencyManagers, err := d.DependencyManagers() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
|
|
||
| if slices.Contains(dependencyManagers, "yarn") { | ||
| return "yarn exec" | ||
| } | ||
|
|
||
| if slices.Contains(dependencyManagers, "npm") { | ||
| return "npm exec " | ||
| } |
There was a problem hiding this comment.
nodeExecPrefix() returns "yarn exec" without a trailing space, but callers concatenate it with the command (e.g. +"next build"), producing an invalid command like yarn execnext build. Return "yarn exec " (or add spacing at the call site) to ensure valid build steps for Yarn.
discovery/build_steps.go
Outdated
| "# Install Pipenv as a global tool", | ||
| "python -m venv /app/.global", | ||
| "pip install pipenv==$PIPENV_TOOL_VERSION", | ||
| "pipenv install", |
There was a problem hiding this comment.
The Pipenv build step uses pipenv install, but Pipenv is only detected via Pipfile.lock (see dependencyManagersMap), and the existing question flow uses pipenv sync to install from the lockfile. Using install can update the lock and produce non-reproducible builds. Switch this step to pipenv sync (or otherwise ensure it installs strictly from the lock).
| "pipenv install", | |
| "pipenv sync", |
| if managePyPath := utils.FindFile( | ||
| d.fileSystem, | ||
| appRoot, | ||
| managePyFile, | ||
| ); managePyPath != "" { | ||
| buildSteps = append( | ||
| buildSteps, | ||
| "# Collect static files", | ||
| fmt.Sprintf("%spython %s collectstatic --noinput", d.pythonPrefix(), managePyPath), | ||
| ) |
There was a problem hiding this comment.
managePyPath is used directly in the collectstatic command, but FindFile() returns a path relative to the repo root (e.g. project/manage.py). If build hooks execute from application_root (common), this becomes the wrong path when application_root != ".". Convert managePyPath to a path relative to appRoot (similar to the existing question implementation) before formatting the command.
Extract project detection logic from question handlers into a new discovery/ package. The Discoverer operates on an fs.FS and provides memoized methods for detecting stack, runtime, dependency managers, build steps, application root, and environment. - Add discovery package with Stack, Type, DependencyManagers, BuildSteps, ApplicationRoot, Environment detection - Add comprehensive tests for all discovery methods - Add Discoverer field to Answers, initialized in WorkingDirectory - Rewrite stack.go to delegate to Discoverer.Stack() instead of inline detection - Add Symfony, Ibexa, Shopware stack constants to platformifier - Add CountFiles utility for runtime detection heuristic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add missing trailing space in nodeExecPrefix() for yarn, which caused "yarn execnext build" instead of "yarn exec next build". - Add missing Rails detection via config.ru, which was dropped during the refactor from question/stack.go to discovery/stack.go. - Add Rails case to discoverType() to return "ruby". - Change pipenv build step from "pipenv install" to "pipenv sync" to match the original behavior (installs from lock file only). - Remove redundant zero-value map initialization in discoverType(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a2655d1 to
c8068a9
Compare
9fd9c6f to
ac58a58
Compare
Summary
Stacked on #261.
Extract project detection logic from question handlers into a new
discovery/package.Discovererstruct operates onfs.FSwith memoized detection methods:Stack(),Type(),DependencyManagers(),BuildSteps(),ApplicationRoot(),Environment()stack.gorewritten to delegate toDiscoverer.Stack()instead of inline detectionSymfony,Ibexa,Shopwarestack constants to platformifierCountFilesutility for runtime detection heuristicAuthor: @akalipetis
🤖 Generated with Claude Code