-
Notifications
You must be signed in to change notification settings - Fork 557
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
docs: Add multi-language entrypoint function page #7351
docs: Add multi-language entrypoint function page #7351
Conversation
Noting that in this case, the original constructor pages for TypeScript and Python have been split, with the first common section extracted to a multi-language page and the remainder left as language-specific content. @helderco and @TomChv please review this to confirm nothing has been lost in translation. |
51c593d
to
d043e1a
Compare
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 makes sense!
Hey! Thanks for the changes I'm going to review this tomorrow! 👀 |
fffc5ca
to
8ff0552
Compare
Note to use latest dagger version to update the output of More context in: |
:::note | ||
If you plan to use constructor fields in other module functions, ensure that they are declared as public. This is because Dagger stores fields using JSON marshalling and private fields are omitted during the marshalling process. As a result, if a field is not declared as public, calling methods that use it will produce unexpected results. | ||
::: |
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.
"declared as public" isn't a thing in Python.
"Marshalling" is a term usually seen in Go. Can use the more general terms "serialization" and "deserialization" (no need for mentioning JSON
too).
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.
Modified
:::info | ||
This language-specific page should be read together with the main [module constructor documentation](../constructor.mdx). | ||
::: |
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 think this should evolve into one page per SDK for more details on various topics, including this one.
It's not necessaritly that this page needs to be read together with the common one if we can make the common section like a sort of basics, and the language-specific more like advanced/detailed information.
In that sense, I think it's ok to have some SDK specific information inside an SDK tab, if it helps to cover the "basics". Even better if we can use collapsed admonitions, like this:
Generally speaking, collapsed admonitions would help keep the content lighter, while still providing additional (and optional) information without having to link to another page in a lot of cases.
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.
For the moment I have removed this note. I would like to separately discuss the topic of a single language-specific page vs a language category. Briefly, for me, the latter has two advantages:
- avoids overloading too much content on one page (eg. this Python-specific constructor page already has a lot of information)
- makes it more visible, as each language-specific page will have a separate sidebar entry
``` | ||
|
||
:::note | ||
Dagger modules have only one constructor. Constructors of custom types are not registered; they are constructed by the function that [chains](./chaining.mdx) them. |
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.
Removed the link to custom types, I assume because that doc merge hasn't been merged yet. But it's already been approved so making a note to not forget adding it here.
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.
Added
|
||
The code listing above is an example of an object that has typed attributes. | ||
|
||
[Learn more about Python module constructors](./python/constructor.mdx). |
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.
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.
Moved to end of page
```python file=./snippets/constructor/python/main.py | ||
``` | ||
|
||
In the Python SDK, the [`@dagger.object_type`](https://dagger-io.readthedocs.io/en/latest/module.html#dagger.object_type) decorator wraps [`@dataclasses.dataclass`](https://docs.python.org/3/library/dataclasses.html), which means that an `__init__()` method is automatically generated, with parameters that match the declared class attributes. |
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 think this would be better in an info
admonition. It's important to know that @object_type
is a @dataclass
. Perhaps even put the "learn more" link here as most of that is about the dataclass thing.
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.
Changed to admonition. Link is added at the end of the page
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.
In "Asynchronous constructor", maybe embolden:
a factory class method which must be named
create
:
It's important to know it has to have that name.
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.
Changed to highlight box
8ff0552
to
e4f156f
Compare
Updated. Can you please review again @helderco ? Thank you! |
Vikram and I looked this, once links are removed and bottom note is combined I think this is ready to ship. |
856fc66
to
71131e8
Compare
Noting here that the language-specific constructor pages have very different examples. These have been merged into this page under separate headings. I'm not sure if these are different because of actual disparity between SDKs or because they were authored at different times. See the preview at https://deploy-preview-7351--devel-docs-dagger-io.netlify.app/manuals/developer/constructor/ I would like @helderco @TomChv @kpenfound to review this once and advise which of the language-specific examples could be made multi-language. I'm not sure if we need to block merging this PR until the above is reviewed and new examples possibly created by the SDK maintainers. WDYT @helderco @levlaz @jpadams @kpenfound ? |
Can I also request that we call this topic "entrypoint functions" and explain that every module has one. The default one is generated automatically and has no arguments; It's possible to write a custom entrypoint function, the mechanism is SDK-specific. |
Done, let me know if further changes needed. I made this change in the title, intro and first section but not in the following language-specific ones since the |
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
This reverts commit dff4791. Signed-off-by: Vikram Vaswani <vikram@dagger.io>
This reverts commit 2f27465. Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
b07d815
to
672a0cb
Compare
Hello, Daggerverse! | ||
``` | ||
|
||
Constructor arguments are documented through their attribute documentation, |
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 inline doc section is not transferred because there is a similar example (object documentation) already provided in the inline documentation page, but lmk if this is not correct @helderco
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 left small comments about the organization of this page.
If you plan to use constructor fields in other module functions, ensure that they are declared as public (in Go and TypeScript). This is because Dagger stores fields using serialization and private fields are omitted during the serialization process. As a result, if a field is not declared as public, calling methods that use it will produce unexpected results. | ||
::: | ||
|
||
## Exclude argument in constructor (Python) |
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 there a way to put this in a specific Python tab? It looks strange to read that if I'm on the TS or Go doc.
And I could miss some details, for example TypeScript specificity is at the bottom so users could miss 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.
Yes, I can try. Can you please let me know if any of the Python examples can be replicated in TypeScript? If yes, can you add the code snippets?
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.
You should be able to do it but just setting default arg in the field and not adding it in the constructor
``` | ||
|
||
:::note | ||
If you plan to use constructor fields in other module functions, ensure that they are declared as public (in Go and TypeScript). This is because Dagger stores fields using serialization and private fields are omitted during the serialization process. As a result, if a field is not declared as public, calling methods that use it will produce unexpected results. |
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 understand that we're multi language but here you mention Go, we should have a specific Go section instead?
Next steps as of 5/31 - @TomChv please take the lead to make decision for this specific comment, so we can move forward with next steps on this PR since Helder is OOO next week. |
The information in this section is only applicable to the Python SDK. | ||
::: | ||
|
||
The opposite is also possible. To define an argument that only exists in |
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.
You could do it by not exposing the @field
inside a variable, this should works
```python file=./snippets/entrypoint/python/factory/main.py | ||
``` | ||
|
||
## Asynchronous constructor (Python) |
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.
Not possible in Typescript without a hack that we don't want to recommand
```python file=./snippets/entrypoint/python/initvar/main.py | ||
``` | ||
|
||
## Complex or mutable defaults (Python) |
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.
You can call new
to constructor a complex object or dag.xxx
for a dagger object inside the constructor
Next steps: @vikram-dagger will take the next steps on this PR.
|
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
Done
Done, except that instead of tabs I'm leaving these as sections with a note that these are only applicable to Python (this is the format already used in other pages) |
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
docs/current_docs/manuals/developer/snippets/entrypoint/go/default-object/main.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
import "context" | ||
|
||
func New( | ||
// +default=dag.Container().From("alpine:3.14.0") |
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.
Error
Error: generate code: template: alias.go.tmpl:76:3: executing "_dagger.gen.go/alias.go.tmpl" at <ModuleMainSrc>: error calling ModuleMainSrc: failed to generate type def code for MyModule: failed to convert constructor to function def: default value "dag.Container().From(\"alpine:3.14.0\")" must be valid JSON: invalid character 'd' looking for beginning of value
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 don't think this is possible? In go you can just have it as +optional
and then in the constructor say
if ctr == nil {
ctr = dag.Container().From("alpine:3.14.0")
}
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.
Updated and ready for review
Signed-off-by: Vikram Vaswani <vikram@dagger.io>
No description provided.