-
Notifications
You must be signed in to change notification settings - Fork 7
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
improvements to besom-cfg #525
Conversation
a477cc1
to
e1c1563
Compare
e1c1563
to
ea877a8
Compare
# Compiles besom-cfg k8s module | ||
compile-cfg-k8s: publish-local-cfg-lib | ||
compile-cfg-k8s: publish-local-cfg-lib publish-local-cfg-containers | ||
just cli packages local kubernetes:4.17.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to have this version fixed here? What about publishing this module to maven? Are we going to always release besom-kubernetes for the latest k8s version and for 4.17.1? Otherwise this module will be unusable when downloaded from maven central, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version is indeed fixed here and in project.scala for cfg-k8s, does it matter though? macro-stuff is going away, codegen is going to generate classes for the user without depending explicitly on besom-kubernetes.
besom-cfg/containers/.scalafmt.conf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file actually used? It looks that our CI uses the .scalafmt.conf
file from the project's root to verify formatting throughout the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do open separate folders as projects in vscode since out scala-compose thing died ;_;
@@ -3,6 +3,11 @@ package besom.cfg | |||
import scala.quoted.* | |||
import besom.json.* | |||
import besom.cfg.internal.* | |||
import besom.util.Validated | |||
import scala.util.control.NoStackTrace | |||
import org.checkerframework.checker.units.qual.A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this line is still to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what the hell, I did kill it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, pushed fix
import scala.util.control.NoStackTrace | ||
import org.checkerframework.checker.units.qual.A | ||
|
||
final val Version = "0.4.0-SNAPSHOT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet another place to remember about when bumping versions 😞
How about using scala-cli's BuildInfo
to keep this up to date with what we have in the module's build definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but no, we're moving away from macros, right?
end Configured | ||
|
||
def resolveConfiguration[A](using c: Configured[A]): A = | ||
c.newInstanceFromEnv() | ||
def resolveConfiguration[A, INPUT: Default](using c: Configured[A, INPUT]): A = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be proper API for users?
Should they then call something like resolveConfiguration[Foo, Map[String, String]]
?
Are we sure this is going to work well when we mix type inference and implicit resolution with overloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good question and I think this was WIP but since we're killing macros-based approach we can ignore this and figure that out later, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
No description provided.