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

fix: Remove dangerous --continue-on-error flag #152

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Aug 18, 2024

While this flag was meant to speed up development, people are starting to abuse it any ignore any errors.
They won't even know that the spec isn't complete due to all the errors.

@nickvergessen
Copy link
Member

nickvergessen commented Aug 18, 2024

I would keep the option, but still make the result != 0, so that CI complains?
But at least you get the full list of your errors

@provokateurin
Copy link
Member Author

I think all the errors that are thrown are important and shouldn't be ignored, except for the missing docs which already have a separate flag that can be used to skip them.

@nickvergessen
Copy link
Member

yeah, which is why it should still error the command and on CI. But it's quite annoying if you have to run the command step by step to reach the next error?

My suggestion would be additionally to your current patch:

diff --git a/generate-spec b/generate-spec
index 0780d47..1108a78 100755
--- a/generate-spec
+++ b/generate-spec
@@ -1024,3 +1024,8 @@ foreach ($scopePaths as $scope => $paths) {
 
        Logger::info('app', 'Generated scope ' . $scope . ' with ' . $pathsCount . ' routes!');
 }
+
+if (Logger::$loggedError) {
+       // Error the run on CI when an error was logged
+       exit(1);
+}
diff --git a/src/Logger.php b/src/Logger.php
index 1af603a..a7cea6a 100644
--- a/src/Logger.php
+++ b/src/Logger.php
@@ -4,6 +4,7 @@ namespace OpenAPIExtractor;
 
 class Logger {
        public static bool $verbose = false;
+       public static bool $loggedError = false;
 
        protected static function log(LoggerLevel $level, string $context, string $text): void {
                print(self::format($level, $context, $text));
@@ -33,10 +34,8 @@ class Logger {
                self::log(LoggerLevel::Warning, $context, $text);
        }
 
-       /**
-        * @throws LoggerException
-        */
        public static function error(string $context, string $text): void {
-               throw new LoggerException(LoggerLevel::Error, $context, $text);
+               self::$loggedError = true;
+               self::log(LoggerLevel::Error, $context, $text);
        }
 }

@provokateurin
Copy link
Member Author

Not necessarily everyone runs it in CI, so the final exit code might be missed.
But I guess your suggestion makes sense, then the flag can still be removed.

@provokateurin provokateurin force-pushed the fix/remove-continue-on-error-flag branch from 06c0c73 to 1d43651 Compare August 19, 2024 09:00
@provokateurin provokateurin merged commit 2153e70 into main Aug 19, 2024
13 checks passed
@provokateurin provokateurin deleted the fix/remove-continue-on-error-flag branch August 19, 2024 09:03
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