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

Exit with code 1 when errors are found #59

Conversation

daniilsapa
Copy link
Collaborator

@daniilsapa daniilsapa commented Jul 17, 2024

Resolves #56

@daniilsapa
Copy link
Collaborator Author

daniilsapa commented Jul 17, 2024

Hi @illright
Could you review it when you have time?

I had to change the autofixing logic a bit to accomplish this task.
Also I noticed several issues with autofix engine and also possible improvements

Issues that I noticed so far:

  • It autofixes with .js files even if a project uses TS
  • Why do we print errors first and then try to autofix them, shouldn't we swap these 2 actions to not report errors that were autofixed?

I saw open tickets to improve the autofix mechanism and realized that this was a separate complex problem within this project.

Possible issues/improvements that I think are worth discussion:

  • What if a file was moved with one fix and then we run another fix that needs to fix imports inside that file? As I see right now the autofix engine does not handle such a case and there would be an error thrown
  • It's better to have some kind of abortion logic, for example, to handle cases when a complex fix is applied partially and then fails

I think we need to manipulate some kind of virtual file system and then when we are sure that a needed fix (or a sequence of fixes, e.g. "move" -> "rename" -> "fix import statements inside") is possible to apply without errors, we commit it to the real FS.

Let's create a separate discussion. Because it feels like a complex problem that needs a lot of discussions

@daniilsapa daniilsapa changed the title Feature/exit with 1 when errors found Exit with code 1 when errors are found Jul 17, 2024
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! As for the auto-fix engine, yes, it's a whole other story. I have some ideas in mind about what to do with it, but let's talk about it in one of the relevant issues.

Could you also add a patch changeset for Steiger please?

@daniilsapa
Copy link
Collaborator Author

Oh, yes, sorry. I forgot to push that commit yesterday

@daniilsapa daniilsapa requested a review from illright July 18, 2024 09:28
@daniilsapa
Copy link
Collaborator Author

@illright
I created a separate discussion where I drafted my vision/ideas of the engine, so we can groom it in one place and not scatter around the auto-fix-related issues. Looking forward to seeing your ideas on the auto-fix engine there!

@illright illright merged commit 79f3552 into feature-sliced:master Jul 19, 2024
7 checks passed
@daniilsapa daniilsapa deleted the feature/exit-with-1-when-errors-found branch July 20, 2024 14:10
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.

Exit with 1 exit code when there are reported errors
2 participants