Skip to content

remove potential leftover data#52

Merged
IMax153 merged 2 commits intoEffect-TS:mainfrom
tatchi:fix-file-remove-error
Sep 25, 2024
Merged

remove potential leftover data#52
IMax153 merged 2 commits intoEffect-TS:mainfrom
tatchi:fix-file-remove-error

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 #

@changeset-bot
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

@tatchi tatchi force-pushed the fix-file-remove-error branch from 22062bd to 8cabe37 Compare September 22, 2024 09:58
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
@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