-
Notifications
You must be signed in to change notification settings - Fork 0
minor fix #1
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
minor fix #1
Conversation
Reviewer's GuideRefines setup behavior to better handle existing domain configurations and non-interactive environments, while rebranding the package from @burgan-tech/vnext-template to @burgan-tech/morph-touch across code, metadata, and CI workflows. Sequence diagram for updated setup behavior in interactive and non_interactive modessequenceDiagram
actor Developer
participant cli as init_js_or_setup_js
participant setup as setup_function
participant cfg as getDomainFromConfig
participant cfgCheck as isDomainFolderConfigured
participant domCheck as isAlreadySetup
participant domName as getDomainName
participant nonInt as isNonInteractive
participant fs as filesystem
Developer->>cli: run morph_touch_setup_or_npx_morph_touch
cli->>setup: invoke setup_function()
setup->>setup: isInstalledAsDependency()
alt installed_as_dependency
setup-->>cli: return (skip setup)
cli-->>Developer: exit
else normal_install
setup->>cfg: getDomainFromConfig()
cfg->>fs: read vnext_config_json
fs-->>cfg: config_or_null
cfg-->>setup: domainFromConfig
alt domain_folder_already_configured
setup->>cfgCheck: isDomainFolderConfigured()
cfgCheck->>fs: check_domain_folder_and_structure
fs-->>cfgCheck: exists_with_vnext_structure
cfgCheck-->>setup: true
setup-->>cli: log_domain_already_configured_and_skip
cli-->>Developer: exit
else not_configured_via_config
setup->>domCheck: isAlreadySetup()
alt touch_or_other_domain_exists
domCheck->>fs: check_touch_and_domain_directories
fs-->>domCheck: vnext_structure_found
domCheck-->>setup: true
setup-->>cli: log_template_already_set_up_and_skip
cli-->>Developer: exit
else fresh_setup
domCheck-->>setup: false
setup->>domName: getDomainName()
domName->>nonInt: isNonInteractive()
alt non_interactive
nonInt-->>domName: true
domName-->>setup: null
setup-->>cli: log_skipping_setup_non_interactive
cli-->>Developer: exit
else interactive
nonInt-->>domName: false
domName->>Developer: prompt_for_domain_name
Developer-->>domName: enter_domain_name
domName-->>setup: domainName
setup-->>cli: perform_replacements_and_create_structure
cli-->>Developer: setup_complete
end
end
end
end
Flow diagram for updated setup function decision logicflowchart TD
A[start_setup_function] --> B{isInstalledAsDependency}
B -->|true| C[return_skip_setup]
B -->|false| D[getDomainFromConfig]
D --> E{has_domain_from_config}
E -->|true| F[isDomainFolderConfigured]
F --> G{domain_folder_exists_with_vnext_structure}
G -->|true| H[log_domain_already_configured_and_skip]
G -->|false| I[proceed_to_already_setup_check]
E -->|false| I
I --> J[isAlreadySetup]
J --> K{touch_or_other_domain_has_vnext_structure}
K -->|true| L[log_template_already_set_up_and_skip]
K -->|false| M[getDomainName]
M --> N[isNonInteractive]
N --> O{non_interactive_environment}
O -->|true| P[return_null_domain_name]
O -->|false| Q[prompt_user_for_domain_name]
P --> R{domainName_is_null}
Q --> S[domainName_from_user]
S --> T[perform_domain_setup_with_domainName]
T --> U[end_setup]
R -->|true| V[log_skipping_setup_non_interactive_with_instructions]
V --> U
R -->|false| T
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Summary |
|---|---|
GitHub Workflows .github/workflows/build-and-publish.yml, .github/workflows/check-sonar.yml |
Updated all package name references from vnext-template to morph-touch in workflow steps, NPM links, and release notes. SonarCloud projectKey updated from burgan-tech_vnext-template to burgan-tech_morph-touch. |
Package Configuration package.json |
Renamed package from @burgan-tech/vnext-template to @burgan-tech/morph-touch; updated bin entries (vnext-template/vnext-setup → morph-touch/morph-touch-setup); refreshed repository, issues, and homepage URLs; added publishable files array; promoted @burgan-tech/vnext-schema@^0.0.23 from devDependencies to production dependencies. |
Setup and Initialization init.js, setup.js |
Updated package references and user-facing CLI messages from vnext-template to morph-touch. Enhanced setup.js with non-interactive mode detection, config-based domain setup via vnext.config.json, and improved existing setup detection logic to skip redundant configuration. |
Scripts and Utilities test.js, validate.js |
Updated console output and log messages to reference morph-touch instead of vnext-template. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- setup.js requires careful review of new non-interactive and config-aware logic, particularly the enhanced
isAlreadySetup(), new helper functions (isNonInteractive(),getDomainFromConfig(),isDomainFolderConfigured()), and early exit conditions in thesetup()function. - Verify that the new domain configuration detection in setup.js correctly reads vnext.config.json and validates folder structures without breaking existing workflows.
- Confirm that production dependency addition of
@burgan-tech/vnext-schema@^0.0.23is intentional and properly versioned. - Most other changes (rebranding, URL updates) are straightforward text replacements.
Poem
🐰 A touch of morph, a name renewed,
From vnext-template, the template flew!
New config flows, no prompts to pain,
The rabbit hops through setup's new lane! ✨
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
master
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/build-and-publish.yml(5 hunks).github/workflows/check-sonar.yml(1 hunks)init.js(2 hunks)package.json(4 hunks)setup.js(5 hunks)test.js(1 hunks)validate.js(1 hunks)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Summary of ChangesHello @middt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on a significant rebranding effort, transitioning the package identity from Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The logic for detecting an already-configured domain (checking for
Schemas/Workflows/Tasksin bothisAlreadySetupandisDomainFolderConfigured) is duplicated; consider extracting a shared helper to avoid divergent behavior over time. - The
@burgan-tech/vnext-schemapackage is now listed in bothdependenciesanddevDependencies; if it’s only needed at build/validation time, you can keep it just indevDependenciesto avoid bloating the runtime install.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for detecting an already-configured domain (checking for `Schemas`/`Workflows`/`Tasks` in both `isAlreadySetup` and `isDomainFolderConfigured`) is duplicated; consider extracting a shared helper to avoid divergent behavior over time.
- The `@burgan-tech/vnext-schema` package is now listed in both `dependencies` and `devDependencies`; if it’s only needed at build/validation time, you can keep it just in `devDependencies` to avoid bloating the runtime install.
## Individual Comments
### Comment 1
<location> `package.json:59` </location>
<code_context>
"hasInstallScript": true,
"license": "MIT",
+ "dependencies": {
+ "@burgan-tech/vnext-schema": "^0.0.23"
+ },
"bin": {
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating @burgan-tech/vnext-schema in both dependencies and devDependencies.
Having the same package/version in both dependencies and devDependencies is redundant and risks them drifting apart over time. Keep it only where it’s needed at runtime (likely dependencies) and remove the duplicate entry.
Suggested implementation:
```
"dependencies": {
"@burgan-tech/vnext-schema": "^0.0.23",
"@burgan-tech/morph-touch": "./init.js",
"morph-touch": "./init.js",
"morph-touch-setup": "./setup.js"
},
```
1. Locate the `"devDependencies"` section in this same `package.json` file.
2. Remove the line that declares `@burgan-tech/vnext-schema` there, e.g. delete:
`"@burgan-tech/vnext-schema": "^0.0.23",`
3. Ensure the surrounding JSON remains valid (commas on previous/next lines adjusted as needed).
</issue_to_address>
### Comment 2
<location> `setup.js:10-16` </location>
<code_context>
if (fs.existsSync('touch')) {
if (fs.existsSync(path.join('touch', 'Schemas')) ||
fs.existsSync(path.join('touch', 'Workflows')) ||
fs.existsSync(path.join('touch', 'Tasks'))) {
return true;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (fs.existsSync('touch') && (fs.existsSync(path.join('touch', 'Schemas')) ||
fs.existsSync(path.join('touch', 'Workflows')) ||
fs.existsSync(path.join('touch', 'Tasks')))) {
return true;
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 3
<location> `setup.js:217-223` </location>
<code_context>
if (fs.existsSync(domainFromConfig)) {
if (fs.existsSync(path.join(domainFromConfig, 'Schemas')) ||
fs.existsSync(path.join(domainFromConfig, 'Workflows')) ||
fs.existsSync(path.join(domainFromConfig, 'Tasks'))) {
return true;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (fs.existsSync(domainFromConfig) && (fs.existsSync(path.join(domainFromConfig, 'Schemas')) ||
fs.existsSync(path.join(domainFromConfig, 'Workflows')) ||
fs.existsSync(path.join(domainFromConfig, 'Tasks')))) {
return true;
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Code Review
This pull request primarily handles the renaming of the package from vnext-template to morph-touch, updating all relevant files and configurations. It also introduces enhancements to the setup script, such as support for non-interactive environments and improved idempotency checks. The changes are generally well-implemented, but I have a couple of suggestions regarding package dependency configuration and code duplication that would improve the overall quality.
| }, | ||
| "dependencies": { | ||
|
|
||
| "@burgan-tech/vnext-schema": "^0.0.23" |
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.
The package @burgan-tech/vnext-schema is now listed as both a dependency and a devDependency. Since it appears to be used only for development and validation scripts (like validate.js), it should remain only in devDependencies. Including it in dependencies will unnecessarily add it to the production installs of projects that use this package.
| if (fs.existsSync(path.join(domainFromConfig, 'Schemas')) || | ||
| fs.existsSync(path.join(domainFromConfig, 'Workflows')) || | ||
| fs.existsSync(path.join(domainFromConfig, 'Tasks'))) { |
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.
The logic to check for a vNext directory structure (i.e., the presence of Schemas, Workflows, or Tasks subdirectories) is duplicated in this function (isDomainFolderConfigured) and also within isAlreadySetup. To improve maintainability and reduce redundancy, this logic should be extracted into a separate helper function.
For example, you could create a function like this:
function hasVNextStructure(dirPath) {
return fs.existsSync(path.join(dirPath, 'Schemas')) ||
fs.existsSync(path.join(dirPath, 'Workflows')) ||
fs.existsSync(path.join(dirPath, 'Tasks'));
}This new helper could then be used in both isDomainFolderConfigured and isAlreadySetup to simplify the code and avoid repetition.
Summary by Sourcery
Rename the package and related tooling from the old vnext-template identity to the new morph-touch identity while improving setup behavior for non-interactive and preconfigured environments.
New Features:
Enhancements:
Build:
CI:
Tests:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.