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

Templating with classes does not fail if expected #284

Closed
StefMa opened this issue Mar 2, 2024 · 5 comments
Closed

Templating with classes does not fail if expected #284

StefMa opened this issue Mar 2, 2024 · 5 comments

Comments

@StefMa
Copy link
Contributor

StefMa commented Mar 2, 2024

Given a similar template as you have in the documentation "How to write a template".

title: String
interactive: Boolean
seats: Int
occupancy: Float
duration: Duration
`abstract`: String
class Event {
-  name: String
+  name: String?
-  year: Int
+  year: Int?
}
event: Event
instructors: Listing<String>
class Session {
  time: Duration
  date: String
}
sessions: Listing<Session>
assistants: Mapping<String, String>

The implementation leaves out the event property:

title = "Pkl: Configure your Systems in New Ways"
interactive = true
seats = 100
occupancy = 0.85
duration = 1.5.h
`abstract` = """
  With more systems to configure, the software industry is drowning in repetitive and brittle configuration files.
  YAML and other configuration formats have been turned into programming languages against their will.
  Unsurprisingly, they don’t live up to the task.
  Pkl puts you back in control.
  """
-event {
-  name = "Migrating Birds between hemispheres"
-  year = 2024
-}
instructors {
  "Kate Sparrow"
  "Jerome Owl"
}
sessions {
  new {
    date = "2/1/2024"
    time = 30.min
  }
  new {
    date = "2/1/2024"
    time = 30.min
  }
}
assistants {
  ["kevin"] = "Kevin Parrot"
  ["betty"] = "Betty Harrier"
}

Expected:
Error with message

Tried to read property event but its value is undefined.

But I get:
Not error, outpout contains

event {
  name = null
  year = null
}

For me looks wrong.
event: Event from the template is not overriden so it should fail.

Fun fact:
If I leave a property within Event non-nullable, it will fail with the correct error message:

Tried to read property name but its value is undefined.
...
The above error occurred when rendering path event.name of module "..."

Another (simpler) example:
Given the following template:

class On {
  first: Int?
  second: Int?
  third: Int?
}

on: On

My template expect to override On.
However, non of the properties of On are required to set.

Implementation:

☝️ No joke, just the amend statement of course 😁

What will be rendered?

on {
  first = null
  second = null
  third = null
}

Technically this behaviour seems to be correct.
It might be the case that pkl will "simply" add the "null class" for you to the implementation.
Because not defining on at all or using on {} will lead to the same output.
The latter (on {}) is perfectly fine and shouldn't print an error,
Nevertheless, I think we should fail here because it behaves differently to other errors that says "value is undefined".

@stackoverflow
Copy link
Contributor

Most non-primitive types in Pkl have a default value. The default value of nullable types is null.
If you want to force people to give a value, you have two options:

  • Make the property non-null. This indicates that it has to be provided and can't be left empty.
  • Make the default an error: first: Int? = throw("error: no value provided for property 'first'")

@StefMa
Copy link
Contributor Author

StefMa commented Mar 4, 2024

Hey @stackoverflow ,

thanks for the answer.

Make the property non-null. This indicates that it has to be provided and can't be left empty.

How do I mark a property as non-null?
Isn't declaring it as on: On enough?
In fact, I have this!
See https://github.com/StefMa/pkl-gha/blob/d266b8b2f9a541774b2f648a1c583d057c931462/GitHubAction.pkl#L139-L149

It also works with build-in classes like String or Int.
In these cases it works:
Screenshot 2024-03-04 at 3 57 43 PM

However, on and jobs will be rendered empty...
Screenshot 2024-03-04 at 3 58 16 PM

Package used:

amends "package://pkg.pkl-lang.org/github.com/stefma/pkl-gha/com.github.action@0.0.1#/GitHubAction.pkl"

@stackoverflow
Copy link
Contributor

How do I mark a property as non-null?
Just remove the ? from the end of the type: first: Int instead of first: Int?

Here's a list of default values for different types, in case you are confused: https://pkl-lang.org/main/current/language-reference/index.html#default-values

Primitive types like String, Int, Boolean, etc have no default values. That's why you get an error if you don't provide them.

@bioball
Copy link
Contributor

bioball commented Mar 4, 2024

Is this invalid output?

on: {}

If this is valid, then there is nothing to do here.

If it this invalid, then you will want to think about what the validation rules are, and define them in your template.

For example, maybe you want to say that at least one sub-property is defined. If so, then you can do:

local const hasAtLeastOneProperty = (it: On) -> it.first != null || it.second != null || it.third != null

on: On(hasAtLeastOneProperty)

Then, Pkl will throw if some property is not defined.

Alternatively, if on: {} is valid, but you would rather omit this from your output altogether if it's empty. In that case, you can just make the property nullable, and have your renderer omit null properties.

For the YAML use-case, it already does omit null properties so nothing else needs to be set.

on: On?

output {
  renderer = new YamlRenderer {}
}

@StefMa
Copy link
Contributor Author

StefMa commented Mar 4, 2024

@stackoverflow I was not aware of the concept of default properties 🤯 thanks for sharing! Know it make sense!

@bioball thanks for the explanation!
Indeed, on {}without a single property set is invalid. Seems I wanna introduce a logic you provided.

Thanks for your support here guys.
Much appreciated.

Going to close it as it everything works as expected.

@StefMa StefMa closed this as not planned Won't fix, can't repro, duplicate, stale Mar 4, 2024
@StefMa StefMa changed the title Templing with classes does not fail if expected Templating with classes does not fail if expected Mar 5, 2024
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

No branches or pull requests

3 participants