first draft of OxApp with extension companion traits #157

lbialy · 2024-06-20T11:33:21Z

No description provided.

lbialy · 2024-06-20T22:24:26Z

@adamw CI crash is unrelated to this code 😮

adamw · 2024-06-21T08:08:03Z

@lbialy yes, already fixed the test: 7a4a73b

adamw · 2024-06-21T08:18:57Z

core/src/test/scala/ox/OxAppTest.scala

+
+ def run(args: Vector[String])(using Ox): ExitCode = Success
+
+ Main1.main(Array.empty)


we could also assert that the result is 0 here

but main is Unit, we check ec to be equal 0 below

adamw · 2024-06-21T08:28:19Z

core/src/main/scala/ox/OxApp.scala

+
+ private[ox] def printStackTrace(t: Throwable): Unit = t.printStackTrace()
+
+ private[OxApp] final def handleRun(args: Vector[String])(using Ox): ExitCode =


is Ox needed here?

ah we need it for run, ok

adamw · 2024-06-21T08:28:35Z

core/src/main/scala/ox/OxApp.scala

+ case Left(fatalErr) => throw fatalErr
+ case Right(exitCode) => exit(exitCode.code)
+
+ private[ox] def exit(code: Int): Unit = System.exit(code)


maybe add a comment that these are private[ox] vs private because of testing

adamw · 2024-06-21T08:37:40Z

core/src/main/scala/ox/OxApp.scala

+
+ private[ox] def mountShutdownHook(thread: Thread): Unit =
+ try Runtime.getRuntime.addShutdownHook(thread)
+ catch case _: IllegalStateException => ()


shouldn't we re-throw the exception if mounding the hook fails?

no, it's thrown only if we're already in shut down of the vm so there's nothing more to do

for reference: CE IOApp only shuts down it's thread pools when it gets ISE here

adamw · 2024-06-21T08:38:39Z

core/src/main/scala/ox/OxApp.scala

+ mountShutdownHook(interruptThread)
+
+ cancellableMainFork.joinEither() match
+ case Left(iex: InterruptedException) => exit(0)


is this success, if the app was interrupted?

highly debatable, some sources treat interruption as failure and return 1 or 130 (128+SIGINT(2)), some return 0 if application was able to interrupt itself and shut down cleanly (as there were no issues during shutdown), it's up to us to decide. JVM itself returns 130 on unhandled sigint.

ok, so here we would treat this as "handled sigint"?

but that's great info, can you put the above in a comment in the code? for future readers :)

ok, I'll even make this configurable

adamw · 2024-06-21T08:40:35Z

core/src/main/scala/ox/OxApp.scala

+
+ def run(using Ox): Unit
+
+ trait WithErrors[E] extends OxApp:


can we use an arbitrary error mode WithErrorMode, and specialise to eithers with WithEither?

is something like:

trait WithErrorMode[E, F[_]](em: ErrorMode[E, F]) extends OxApp: override def run(args: Vector[String])(using ox: Ox): ExitCode = val result = run(args.toList) if em.isError(result) then ExitCode.Failure(1) else ExitCode.Success def run(args: List[String])(using Ox): F[ExitCode] abstract class WithEitherErrors[E] extends WithErrorMode(EitherMode[E]()): type EitherScope[Err] = Label[Either[Err, ExitCode]] override final def run(args: List[String])(using ox: Ox): Either[E, ExitCode] = either[E, ExitCode](label ?=> run(args.toVector)(using ox, label)) def run(args: Vector[String])(using Ox, EitherScope[E]): ExitCode

what you had in mind?

mind the change of Vector to List in args because we have conflicting signatures so there's that, we could do a disambiguation by always-provided implicit but that's even more... arcane

also without handleErrors there's no translation of error into exit code

it could be also:

trait WithErrorMode[E, F[_]](em: ErrorMode[E, F]) extends OxApp: override def run(args: Vector[String])(using ox: Ox): ExitCode = val result = run(args.toList) if em.isError(result) then try handleError(em.getError(result)) catch case NonFatal(e) => e.printStackTrace() ExitCode.Failure(1) else ExitCode.Success def handleError(e: E): ExitCode def run(args: List[String])(using Ox): F[ExitCode]

and F[ExitCode] doesn't make much sense in case where we have Label[Either[E, Nothing]] in scope as it means that errors are to be delivered with .ok() / break() so unwrapped ExitCode as retval

hm the Vector/List is also hacky. Maybe we simply need a different name. We've got fork & forkError for the fork-with-error-mode variant, maybe here we could keep a similar convention? (run & runError). Or come up with better names for both :)

another nitpick: I'd avoid using "scope" in names, as we already have concurrency scopes etc. Maybe rename EitherScope -> EitherError?

also, does : F[ErrorCode] and ErrorCode have the same erasure? Isn't the first erased to Object, making the signatures different?

[error] -- [E120] Naming Error: /Users/lbialy/Projects/foss/ox/core/src/main/scala/ox/OxApp.scala:97:8 [error] 97 | def run(args: Vector[String])(using Ox): F[ExitCode] [error] | ^ [error] |Double definition: [error] |override def run(args: Vector[String])(using ox: ox.Ox): ox.ExitCode in trait WithErrorMode at line 90 and [error] |def run(args: Vector[String])(using x$2: ox.Ox): F[ox.ExitCode] in trait WithErrorMode at line 97 [error] |have matching parameter types.

Unfortunately this is illegal.

Ah right, signature is only name+parameters, not the return type

adamw · 2024-06-21T08:40:54Z

Looks great! :) Some docs would be good :)

hearnadam · 2024-06-30T05:49:37Z

core/src/main/scala/ox/OxApp.scala

+ case Left(fatalErr) => throw fatalErr
+ case Right(exitCode) => exit(exitCode.code)
+
+ private[ox] def exit(code: Int): Unit = System.exit(code)


Doesn't it make sense for this function to accept ExitCode, not Int?

yeah, it does

hearnadam · 2024-06-30T05:50:44Z

core/src/main/scala/ox/OxApp.scala

+ case Failure(exitCode: Int = 1) extends ExitCode(exitCode)
+
+trait OxApp:
+ def main(args: Array[String]): Unit =


adamw · 2024-07-02T12:30:44Z

Looks good! I think we're just missing the documentation.

Only thing I think I would change, is renaming the run method instead of using the Vector/List signatures trick

lbialy · 2024-07-02T15:41:29Z

The signature that requires List[String] is only present in WithErrorMode trait and is expected to be used in the child trait so yeah, can be renamed to runWithErrors.

adamw · 2024-07-02T17:16:50Z

@lbialy yeah this should work fine, also will be consistent with supervisedError and forkError that we already have

lbialy · 2024-07-09T08:06:15Z

@adamw this is now ready :)

hearnadam · 2024-07-09T08:09:08Z

core/src/main/scala/ox/OxApp.scala

+ /** This value is returned to the operating system as the exit code when the app receives SIGINT and shuts itself down gracefully.
+ * Default value is `ExitCode.Success` (0). JVM itself returns code `130` when it receives `SIGINT`.
+ */
+ gracefulShutdownExitCode: ExitCode = ExitCode.Success


I generally prefer to avoid default parameters for case classes - they are hard to change later. But doesn't seem likely to change.

Yes, since we have the default instance let's move this there

lbialy mentioned this pull request Jun 20, 2024

Proposal: provide an OxApp trait as main entry point for app #152

Open

adamw reviewed Jun 21, 2024

View reviewed changes

hearnadam reviewed Jul 1, 2024

View reviewed changes

lbialy added 5 commits July 8, 2024 13:03

first draft of OxApp with extension companion traits

15c3de5

maybe like this?

7ed38e5

added scaladocs, fixed things from review

7914adc

rename the contentious run method to runWithErrors

05bc9ba

added docs

eb9c897

lbialy force-pushed the ox-app-entry-point branch from 77c9b41 to eb9c897 Compare July 8, 2024 22:57

hearnadam approved these changes Jul 9, 2024

View reviewed changes

adamw added 3 commits July 9, 2024 17:19

Cosmetics

d537c91

Remove default from AppSettings,gracefulShutdownExitCode

b27f10b

Docs

b124525

adamw merged commit f3332bd into softwaremill:master Jul 9, 2024
5 checks passed


		def run(args: Vector[String])(using Ox): ExitCode = Success

		Main1.main(Array.empty)


		private[ox] def printStackTrace(t: Throwable): Unit = t.printStackTrace()

		private[OxApp] final def handleRun(args: Vector[String])(using Ox): ExitCode =

first draft of OxApp with extension companion traits #157

first draft of OxApp with extension companion traits #157

Conversation

lbialy commented Jun 20, 2024

lbialy commented Jun 20, 2024

adamw commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbialy Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Jul 2, 2024

lbialy commented Jul 2, 2024

adamw commented Jul 2, 2024

lbialy commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbialy Jun 27, 2024 •

edited

Loading