Skip to content
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

remove potential leftover data #52

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Conversation

tatchi
Copy link
Contributor

@tatchi tatchi commented Sep 22, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Although I said no to the changeset and nix, I still got some content related to them like the flake.nix file or changeset patchedDependencies

Related

  • Related Issue #
  • Closes #

Copy link

changeset-bot bot commented Sep 22, 2024

🦋 Changeset detected

Latest commit: 88bcdca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-effect-app Patch

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

@@ -249,7 +249,7 @@ function createTemplate(config: TemplateConfig) {
// Handle user preferences for Nix flakes
if (!config.projectType.withNixFlake) {
yield* Effect.forEach(
[".envrc", "flake.lock", "flake.nix"],
[".envrc", "flake.nix"],
Copy link
Contributor Author

@tatchi tatchi Sep 22, 2024

Choose a reason for hiding this comment

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

This effect was failing because flake.lock doesn't exist, so we're not going past this code in this case. I feel like this is not the best fix; we should try to avoid this kind of situation in the future.

I guess we could pass { force: true } to fs.remove to ignore the exception if the path does not exist. Or handle it at the effect level and use something like Effect.ignore/ignoreLogged. Not sure if ignoring the error is the best solution though, open to any better idea ?

Copy link
Member

Choose a reason for hiding this comment

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

@tatchi - thank you! I think either solution is fine, but I feel like I would probably use Effect.ignore for this case.

Either way, this PR looks good for this particular issue. If you want to add Effect.ignores in the relevant places as part of this PR let me know. Otherwise also let me know if you're happy with this as is and I'll get this merged 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we basically add Effect.ignore to every place where we use fs.ignore ?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can start with just adding it to places where we attempt to remove files.

@IMax153 IMax153 merged commit 5f92083 into Effect-TS:main Sep 25, 2024
12 checks passed
@IMax153
Copy link
Member

IMax153 commented Sep 25, 2024

Thanks @tatchi !

@github-actions github-actions bot mentioned this pull request Sep 25, 2024
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.

2 participants