Skip to content

(cli): Indent-Based Code Formatter #454

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

Merged
merged 21 commits into from
Jun 16, 2025

Conversation

luke0408
Copy link
Contributor

Apologies for the late PR. Due to personal circumstances, I haven't been able to code much over the past two months. I’ve been working on this intermittently in my spare time, so I’m only able to submit it now. Thank you for your patience.

Added Features

As discussed in #290 , I implemented a lightweight code formatter using indent-string and detect-indent, as an alternative to Prettier.

Implementation Example

export async function insertWithIndent(
  content: string,
  placeholder: string,
  code: string,
) {
  try {
    const [detectIndent, indentString] = await Promise.all([
      import("detect-indent"),
      import("indent-string"),
    ]);

    const indent = detectIndent.default(content);
    const indentedCode = indentString.default(code, indent.amount, { indent: " " });

    return content.replace(placeholder, indentedCode);
  } catch {
    return content;
  }
}

How It Works

  • detect-indent determines the indentation style from the given content (the original text to be modified).
  • indent-string then applies that indentation to the code (the content to insert).
  • Finally, placeholder in the original content is replaced with the indented code.

Question

I'm unsure whether it would be appropriate to apply this replacement logic as a utility function like the one above. If so, I’d appreciate any suggestions on how to abstract this part cleanly.

@luke0408 luke0408 changed the title Indent-Based Code Formatter (cli): Indent-Based Code Formatter May 26, 2025
@samchon samchon requested review from sunrabbit123 and ryoppippi and removed request for sunrabbit123 May 27, 2025 05:52
@sunrabbit123
Copy link
Member

thank you for your contributing
if this code replace prettier, we don't use dynamic import in this code because this feature will be not a optionalDependency

@@ -14,3 +14,31 @@ describe("formatWithPrettier", () => {
expect(result).toBe(content);
});
});

describe("insertWithIndent", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test code looks good

Copy link
Contributor

@ryoppippi ryoppippi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! @luke0408
the code looks good!!

I'd like to change a few things.

  • remove prettier from package.json. we don't need them anymore
  • remove implementation of prettier and replace them in commands/start.ts
  • add new package names in build.config.ts, otherwise unbuild operation fails. you can check by executing pnpm run build

@luke0408
Copy link
Contributor Author

thank you for your contributing if this code replace prettier, we don't use dynamic import in this code because this feature will be not a optionalDependency

I'll update this part before the work is finalized and merged. :)

@luke0408
Copy link
Contributor Author

Hi! @luke0408 the code looks good!!

I'd like to change a few things.

  • remove prettier from package.json. we don't need them anymore
  • remove implementation of prettier and replace them in commands/start.ts
  • add new package names in build.config.ts, otherwise unbuild operation fails. you can check by executing pnpm run build

So that means the overall code structure looks good—glad to hear that! I was a bit concerned since the structure differs from the original, but now I feel more confident to move on to the next step.

I'll make the changes you mentioned shortly.

@luke0408
Copy link
Contributor Author

@sunrabbit123 @ryoppippi

Also, would it be okay to add dev.nix to the .gitignore file?

I'm currently working in the "Firebase Studio" environment, and in order to use pnpm, I need to configure it through a dev.nix file so that the container installs everything properly.

I think adding it to .gitignore would help prevent any potential conflicts with other environments. What do you think?

@sunrabbit123
Copy link
Member

Also, would it be okay to add dev.nix to the .gitignore file?

I'm currently working in the "Firebase Studio" environment, and in order to use pnpm, I need to configure it through a dev.nix file so that the container installs everything properly.

I think adding it to .gitignore would help prevent any potential conflicts with other environments. What do you think?

I'm fine with it.
The idea that each development environment can be replaced by a .gitignore file is very appealing.
However, I don't clearly remember the context and I recall having discussed this with ryoppippi before, and it was rejected.

Therefore, I think we need to check ryoppippi's opinion again.

@luke0408
Copy link
Contributor Author

While running pnpm run build and testing after modifying the code that previously used Prettier, I encountered an error in my environment saying that promisify and exec could not be found.

Could this be an issue with my environment?

@ryoppippi
Copy link
Contributor

ryoppippi commented May 27, 2025

Hi @luke0408

I'll update this part before the work is finalized and merged. :)

I think you can include the change in this PR.

Anything merged into main in this repository may be published at any time. It would also be nice if you could verify that start.ts works well without prettier as this PR is intended to replace prettier.

I think adding it to .gitignore would help prevent any potential conflicts with other environments. What do you think?

I don't think it is a good idea :(
Nix is awesome! Actually one of the repositories in wrtnlabs are maintained with Nix :). However, this project is not maintained by Nix and nobody uses Nix for this project. If you want to exclude your personal files in the project, just edit .git/info/exclud to ignore the files

While running pnpm run build and testing after modifying the code that previously used Prettier, I encountered an error in my environment saying that promisify and exec could not be found.

On the CI environment, build process is executed successfully. I think it is your environmental issue. If you create an issue with reproduction environment, I'll take a look 😄

@sunrabbit123
Copy link
Member

imo it's not draft, can i click the 'Ready for review' button?

@ryoppippi
Copy link
Contributor

@luke0408 let me know when you done with this PR.

@luke0408
Copy link
Contributor Author

@ryoppippi

Thank you for your patience.
To be honest, there’s not much work left to do, and I believe it can be finished quickly.

However, due to issues with my development environment, I’m not sure when I’ll be able to continue working on it. As you may know, I’m currently not in a situation where I can freely work, and on top of that, I’m having problems with my internet connection, which has caused the work to come to a halt.

At this point, all I can do is reply via my phone.

If the task needs to be completed sooner, someone else may need to take over the branch and continue the work.

@luke0408 luke0408 marked this pull request as ready for review June 12, 2025 08:45
@ryoppippi
Copy link
Contributor

Thanks I'll fix it with vibe coding!

Move detect-indent and indent-string from devDependencies to dependencies
since they are imported at runtime in the insertWithIndent function.
@ryoppippi ryoppippi requested a review from Copilot June 13, 2025 22:07
@ryoppippi ryoppippi force-pushed the feat/indent-based-formatter branch from 3142332 to 7aa78c5 Compare June 13, 2025 22:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the Prettier-based formatting step with a lightweight indent-based inserter for code snippets, removes the Prettier dependency, and updates all usages and tests accordingly.

  • Introduce insertWithIndent utility and deprecate formatWithPrettier
  • Update connector and CLI command logic to use indent-based insertion
  • Adjust tests and package deps (added detect-indent & indent-string)

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/cli/src/utils.ts Added insertWithIndent and removed Prettier formatter function
packages/cli/src/utils.test.ts Swapped out Prettier tests for insertWithIndent and updated imports
packages/cli/src/connectors.ts Updated insertCodeIntoAgenticaStarter to use new insert helper
packages/cli/src/commands/start.ts Removed Prettier formatting from generated files
packages/cli/package.json Removed Prettier optional dep; added detect-indent & indent-string
packages/cli/build.config.ts Excluded new indent deps from bundle
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)

packages/cli/src/utils.ts:23

  • Update the doc comment to clearly state that this function inserts a code block at the placeholder with preserved indentation and returns the original content unchanged if the placeholder isn't found or on error.
* Format the given content with indent-string.

packages/cli/src/utils.test.ts:11

  • The test description doesn't match the assertion (which verifies insertion). Rename to something like should insert code with correct indentation at the placeholder.
it("should return the same content if indent is not available", async () => {

@ryoppippi ryoppippi removed the request for review from sunrabbit123 June 13, 2025 22:10
Copy link

pkg-pr-new bot commented Jun 13, 2025

Open in StackBlitz

@agentica/benchmark

npm i https://pkg.pr.new/wrtnlabs/agentica/@agentica/benchmark@454

@agentica/chat

npm i https://pkg.pr.new/wrtnlabs/agentica/@agentica/chat@454

agentica

npm i https://pkg.pr.new/wrtnlabs/agentica@454

@agentica/core

npm i https://pkg.pr.new/wrtnlabs/agentica/@agentica/core@454

create-agentica

npm i https://pkg.pr.new/wrtnlabs/agentica/create-agentica@454

@agentica/rpc

npm i https://pkg.pr.new/wrtnlabs/agentica/@agentica/rpc@454

@agentica/vector-selector

npm i https://pkg.pr.new/wrtnlabs/agentica/@agentica/vector-selector@454

commit: 4a157e1

@ryoppippi
Copy link
Contributor

CI fails but it is really fragile. it is not because of the implementation in this repo. that's so weird

@ryoppippi
Copy link
Contributor

@luke0408
Once again thank you for your contribution.
I think it works fine, but we need to fix issue for cloning template.

I am no longer with this organisation today and I believe that someone else will take this over.

@sunrabbit123
Copy link
Member

thank you for your contributing!
i will merge it because test is passed!
thanks!

@sunrabbit123 sunrabbit123 merged commit 1a8c763 into wrtnlabs:main Jun 16, 2025
13 of 15 checks passed
@sunrabbit123
Copy link
Member

oops, i miss to modify commit title...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants