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

improvements to besom-cfg #525

Merged
merged 10 commits into from
Oct 2, 2024
Merged

improvements to besom-cfg #525

merged 10 commits into from
Oct 2, 2024

Conversation

lbialy
Copy link
Collaborator

@lbialy lbialy commented Jun 24, 2024

No description provided.

@lbialy lbialy force-pushed the improve-besom-cfg branch 3 times, most recently from a477cc1 to e1c1563 Compare August 29, 2024 10:48
@lbialy lbialy force-pushed the improve-besom-cfg branch from e1c1563 to ea877a8 Compare October 2, 2024 10:32
@lbialy lbialy requested a review from prolativ October 2, 2024 12:30
# 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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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"
Copy link
Collaborator

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?

Copy link
Collaborator Author

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 =
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@lbialy lbialy merged commit 16bd114 into main Oct 2, 2024
2 checks passed
@lbialy lbialy deleted the improve-besom-cfg branch October 2, 2024 20:01
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