feat: rebuild the cli package #207
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's Guide by SourceryThis pull request rebuilds the CLI package, removing several packages and files while adding new functionality to the utils package. The changes focus on environment management, file system operations, and package restructuring. Architecture diagram for CI/CD pipeline changesgraph TD;
A[CI/CD Pipeline] -->|Removed| B[SDK Next Package]
A -->|Removed| C[SDK Config Package]
A -->|Retained| D[SDK Cli Package]
A -->|Retained| E[SDK The Graph Package]
A -->|Retained| F[SDK Hasura Package]
A -->|Retained| G[SDK JS Package]
Class diagram for the new environment managementclassDiagram
class Environment {
+loadEnv() Promise<DotEnv>
+loadEnvironmentEnv(validateEnv: boolean, environment?: string) Promise<DotEnv | DotenvParseOutput>
}
class FileSystem {
+projectRoot() Promise<string>
}
class Validation {
+validate<T>(schema: T, value: unknown): T["_output"]
}
class DotEnvSchema {
+SETTLEMINT_ENVIRONMENT: string
+SETTLEMINT_INSTANCE: UrlSchema
+SETTLEMINT_ACCESS_TOKEN: AccessTokenSchema
}
Environment --> FileSystem : uses
Environment --> Validation : uses
Environment --> DotEnvSchema : uses
FileSystem --> "1" ProjectRoot : returns
Validation --> "1" DotEnvSchema : validates
DotEnvSchema --> "1" AccessTokenSchema
DotEnvSchema --> "1" UrlSchema
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @roderik - I've reviewed your changes - here's some feedback:
Overall Comments:
- This pull request involves significant restructuring, particularly in the CLI package. Could you provide more context about the motivation behind these changes and their expected impact on the project's functionality and architecture?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import { deepmerge } from "deepmerge-ts"; | ||
| import { loadEnvironmentEnv } from "./load-env.js"; | ||
|
|
||
| export async function writeEnv(env: DotEnv, environment: string) { |
Contributor
There was a problem hiding this comment.
suggestion: Consider adding error handling to writeEnv function
The writeEnv function doesn't have any error handling. Consider adding try-catch blocks to handle potential file system errors when writing to the .env file.
export async function writeEnv(env: DotEnv, environment: string) {
try {
// Existing function implementation here
} catch (error) {
console.error(`Error writing .env file for ${environment}:`, error);
throw error;
}
}
| * @returns A promise that resolves to the environment variables, either validated or raw. | ||
| * @throws Will throw an error if validation fails when validateEnv is true. | ||
| */ | ||
| export async function loadEnvironmentEnv<T extends boolean = true>( |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider restructuring the loadEnvironmentEnv function to improve code organization and readability.
While the loadEnvironmentEnv function handles a complex task, it can be restructured to improve readability and maintainability without losing functionality. Here's a suggested approach:
- Separate file path resolution:
function resolveEnvFilePaths(projectDir: string, environment?: string): string[] {
return [
`.env.${process.env.NODE_ENV}.local`,
".env.local",
...(environment ? [`.env.${environment}.local`, `.env.${environment}`] : []),
`.env.${process.env.NODE_ENV}`,
".env",
].map((file) => join(projectDir, file));
}- Simplify the main function:
export async function loadEnvironmentEnv<T extends boolean = true>(
validateEnv: T,
environment?: string
): Promise<T extends true ? DotEnv : DotenvParseOutput> {
const projectDir = await projectRoot();
const paths = resolveEnvFilePaths(projectDir, environment);
let { parsed } = dotenvx.config({ path: paths });
parsed = parsed || {};
const envToUse = environment || parsed.SETTLEMINT_ENVIRONMENT;
if (envToUse && envToUse !== environment) {
return loadEnvironmentEnv(validateEnv, envToUse);
}
return validateEnv ? validate(DotEnvSchema, parsed) : parsed;
}This restructuring:
- Separates the file path resolution logic
- Reduces nesting in the main function
- Maintains the existing functionality
- Improves readability by making the flow more linear
Consider adding comments to explain the purpose of each step, especially the environment fallback logic. This will make the code more self-documenting and easier to maintain in the future.
| const quote = /[\s"'#]/; | ||
|
|
||
| function stringifyPair([key, val]: [string, unknown]): string { | ||
| if (!val) return `${key}=""`; |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
Suggested change
| if (!val) return `${key}=""`; | |
| if (!val) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
📦 Packages
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Rebuild the CLI package by removing numerous commands and libraries, and enhance the utils package with new environment management features. Update the CI workflow to stop publishing certain SDK packages.
New Features:
Enhancements:
CI: