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

Improve None for Optional Defaults Handling #251

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

andyglow
Copy link
Owner

@andyglow andyglow commented Feb 1, 2022

@codecov-commenter
Copy link

Codecov Report

Merging #251 (3156e6a) into master (f0f2085) will decrease coverage by 0.02%.
The diff coverage is 76.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   76.16%   76.13%   -0.03%     
==========================================
  Files          65       65              
  Lines        1674     1777     +103     
  Branches       61       58       -3     
==========================================
+ Hits         1275     1353      +78     
- Misses        399      424      +25     
Impacted Files Coverage Δ
api/src/main/scala/json/Json.scala 0.00% <0.00%> (ø)
...thub/andyglow/json/LowPriorityArrayImplicits.scala 100.00% <ø> (ø)
...cala-2.13/com/github/andyglow/scalamigration.scala 50.00% <ø> (ø)
...n/scala/com/github/andyglow/json/ValueSchema.scala 70.00% <ø> (ø)
...in/scala/com/github/andyglow/json/comparison.scala 61.53% <0.00%> (+23.07%) ⬆️
...scala/com/github/andyglow/jsonschema/AsValue.scala 100.00% <ø> (ø)
...om/github/andyglow/jsonschema/AsValueBuilder.scala 100.00% <ø> (ø)
...scala/com/github/andyglow/jsonschema/package.scala 0.00% <0.00%> (ø)
core/src/main/scala/json/schema/Version.scala 80.00% <ø> (ø)
...cala/com/github/andyglow/jsonschema/Macroses.scala 70.00% <0.00%> (-7.78%) ⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07582fa...3156e6a. Read the comment docs.

@bjornbak
Copy link

bjornbak commented Feb 9, 2022

@andyglow Sorry for the slow response. Been down with corona.

The corner I show cased in my ticket is only visible when I do .asCirce so I add to class AsCirceSpec:

  property("Check that Schema.asCirce translates default None") {
    import io.circe.generic.auto._
    import json.schema.Version.Draft07
    import AsCirce._
    import com.github.andyglow.jsonschema.WrappingObject.NoneForDefaultModelsCase4

    json.Json
      .schema[List[NoneForDefaultModelsCase4]]
      .asCirce(Draft07(id = ""))
      .toString().replaceAll("\\s", "")  shouldBe """"""
  }

and even then I seems to only happen when the case classes are wrapped in an object like:

object WrappingObject {
  case class NoneForDefaultModelsCase4Inner(name: String, innerLevel2: Option[String] = None)

  case class NoneForDefaultModelsCase4(name: String, inner: Option[NoneForDefaultModelsCase4Inner] = None)
}

I am not allowed to push to the PR to demo.

@oleksandr-balyshyn
Copy link

oleksandr-balyshyn commented Mar 9, 2022

I have the same issue (related to .asCirce)

and even then it seems to only happen when the case classes are wrapped in an object like

and it is reproducible even for the case of definition a case class on the root level without wrapping object

@ConfiguredJsonCodec
case class ExtractRule(
  rule: CommonExtractRule,
  defaults: Option[UriDefaults] = None
) extends UriRule

@ConfiguredJsonCodec
case class UriDefaults(
  host: Option[String] = None,
  port: Option[Int] = None,
  scheme: String = "https"
)

@bjornbak
Copy link

@oleksandr-balyshyn Did you find a solution or workaround?

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.

4 participants