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.

Sign up for GitHub

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

Jump to bottom

Remove mentions of controller..can choice syntax in 2.8.0 docs #453

Merged
merged 11 commits into from
Sep 20, 2023
Merged

Remove mentions of controller..can choice syntax in 2.8.0 docs #453

merged 11 commits into from
Sep 20, 2023

Conversation

akrmn
Copy link
Contributor

@akrmn akrmn commented Sep 4, 2023 •
edited
Loading

Part of digital-asset/daml#17361

Daml PRs: digital-asset/daml#17362, digital-asset/daml#17390
DA-GHC PR: digital-asset/ghc#171
docs.daml.com PR: #453

TODO

@akrmn akrmn changed the title Remove mentions of controller..can syntax in 2.8.0 docs Remove mentions of controller..can choice syntax in 2.8.0 docs Sep 4, 2023
This was referenced Sep 4, 2023
Merged
Merged
@akrmn akrmn requested review from carrielaben-da, hrischuk-da and a team September 4, 2023 14:18
@akrmn akrmn marked this pull request as ready for review September 4, 2023 14:18
akrmn
akrmn commented Sep 4, 2023
View reviewed changes
docs/2.8.0/docs/daml/reference/choices.rst Outdated
Comment on lines 14 to 17
.. literalinclude:: ../code-snippets/Reference.daml
:language: daml
:start-after: -- start choice-first choice name snippet
:end-before: -- end choice-first choice name snippet
:caption: Option 1 for specifying choices: choice name first

.. literalinclude:: ../code-snippets/Reference.daml
:language: daml
:start-after: -- start controller-first choice name snippet
:end-before: -- end controller-first choice name snippet
:caption: Option 2 for specifying choices (deprecated syntax): controller first

:start-after: -- start new choice name snippet
:end-before: -- end new choice name snippet
Copy link
Contributor Author

@akrmn akrmn Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

these new snippets will likely fail until the daml PR that adds them is merged into main

akrmn
akrmn commented Sep 4, 2023
View reviewed changes
docs/2.6.5/docs/daml/reference/choices.rst
@@ -56,7 +56,6 @@ Choice Name

- The name of the choice. Must begin with a capital letter.
- If you're using choice-first, preface with ``choice``. Otherwise, this isn't needed.
- Must be unique in your project. Choices in different templates can't have the same name.
Copy link
Contributor Author

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 has ever been true, at least not since I joined

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK It's true within a file/module, but not across a project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I've updated to reflect this

@akrmn akrmn force-pushed the akrmn/no-controller-can branch from 3a5ed5a to 38a77b9 Compare September 5, 2023 09:01
carrielaben-da
carrielaben-da approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@carrielaben-da carrielaben-da left a comment

Choose a reason for hiding this comment

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

LGTM

docs/2.8.0/docs/daml/reference/choices.rst Outdated Show resolved Hide resolved
docs/2.8.0/docs/daml/reference/choices.rst Outdated Show resolved Hide resolved
docs/2.8.0/docs/daml/reference/structure.rst Outdated Show resolved Hide resolved
@akrmn
Copy link
Contributor Author

akrmn commented Sep 5, 2023

thanks for your feedback @carrielaben-da

@akrmn akrmn force-pushed the akrmn/no-controller-can branch from 8359134 to 52d0334 Compare September 8, 2023 11:46
@akrmn akrmn enabled auto-merge (squash) September 8, 2023 11:46
@akrmn akrmn disabled auto-merge September 8, 2023 11:46
@akrmn akrmn enabled auto-merge (squash) September 8, 2023 11:46
@akrmn
Copy link
Contributor Author

akrmn commented Sep 8, 2023

IIUC, this will only build successfully once digital-asset/daml#17362 (digital-asset/daml@1bee52e) is released and we update 2.8.0/versions.json to match:

docs.daml.com/docs/2.8.0/versions.json

Line 2 in 52d0334

"daml": "2.7.0-snapshot.20230719.11983.0.vd999a21a",

on the flipside, that means we don't really need to keep the old snippets around, so I'll be removing them next week

@akrmn akrmn force-pushed the akrmn/no-controller-can branch from 52d0334 to d3fb57e Compare September 12, 2023 08:38
@akrmn akrmn mentioned this pull request Sep 12, 2023
Merged
@akrmn akrmn disabled auto-merge September 12, 2023 08:43
@akrmn akrmn force-pushed the akrmn/no-controller-can branch from d3fb57e to 3062ec0 Compare September 13, 2023 17:04
@akrmn
Copy link
Contributor Author

akrmn commented Sep 13, 2023

Updated 2.8.0/versions.json:daml to https://github.com/digital-asset/daml/releases/tag/v2.8.0-snapshot.20230908.12101.0.v1bee52e4

@akrmn akrmn enabled auto-merge (squash) September 13, 2023 17:05
akrmn
akrmn commented Sep 13, 2023
View reviewed changes
docs/2.8.0/versions.json Outdated
@@ -1,5 +1,5 @@
{
"daml": "2.7.0-snapshot.20230719.11983.0.vd999a21a",
"daml": "cbcaca974ac24c69246c08f167f20b759ebfade6",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I messed up, digital-asset/daml#17390 isn't part of today's release. do I have to wait for the next release or is it okay to point to (the current commit of) main?

@akrmn akrmn disabled auto-merge September 13, 2023 18:02
garyverhaegen-da
garyverhaegen-da reviewed Sep 14, 2023
View reviewed changes
docs/2.8.0/versions.json Outdated
@@ -1,5 +1,5 @@
{
"daml": "2.7.0-snapshot.20230719.11983.0.vd999a21a",
"daml": "cbcaca974ac24c69246c08f167f20b759ebfade6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"daml": "cbcaca974ac24c69246c08f167f20b759ebfade6",
"daml": "2.8.0-snapshot.20230912.12110.0.v788fccd9",

These must match folders on Artifactory, so using git commits won't work here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I didn't see this last week and thought I had to wait until a snapshot release, now I see that the snapshots on Artifactory are enough. Oh well, lesson learned for the next time :)

akrmn and others added 10 commits September 20, 2023 15:34
@akrmn akrmn force-pushed the akrmn/no-controller-can branch from 81b625c to 938f592 Compare September 20, 2023 14:11
@akrmn akrmn requested a review from a team as a code owner September 20, 2023 14:11
@moisesackerman-da moisesackerman-da enabled auto-merge (squash) September 20, 2023 14:13
@moisesackerman-da moisesackerman-da merged commit e025bd9 into main Sep 20, 2023
@moisesackerman-da moisesackerman-da deleted the akrmn/no-controller-can branch September 20, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moisesackerman-da moisesackerman-da moisesackerman-da left review comments

@garyverhaegen-da garyverhaegen-da garyverhaegen-da left review comments

@carrielaben-da carrielaben-da carrielaben-da approved these changes

@hrischuk-da hrischuk-da Awaiting requested review from hrischuk-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akrmn @garyverhaegen-da @moisesackerman-da @carrielaben-da