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

feature/#450 added doc for get_entities_by_config_id #443

Merged
merged 3 commits into from
May 16, 2023

Conversation

toan-quach
Copy link
Member

No description provided.

@toan-quach toan-quach requested review from FabienLelaquais, jrobinAV and trgiangdo and removed request for FabienLelaquais May 8, 2023 03:30
@toan-quach toan-quach self-assigned this May 8, 2023
Copy link
Member

@trgiangdo trgiangdo left a comment

Choose a reason for hiding this comment

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

I have some comments

docs/manuals/core/entities/data-node-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/data-node-mgt.md Outdated Show resolved Hide resolved
trgiangdo

This comment was marked as duplicate.

docs/manuals/core/entities/data-node-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/data-node-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/pipeline-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/scenario-cycle-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/task-mgt.md Outdated Show resolved Hide resolved
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

Minor form changes proposed.

@@ -45,7 +45,10 @@ passing the data node id as parameter:
# data_node == data_node_retrieved
```

The data nodes that are part of a **scenario**, **pipeline** or **task** can be directly accessed as attributes:
# Get data nodes by config_id
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the other changes, you should remove the underscore.

# Get data nodes by config_id

The data nodes that are part of a **scenario**, **pipeline** or **task** can be directly accessed as attributes by
using its config_id:
Copy link
Member

Choose a reason for hiding this comment

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

"using its config_id" -> "using their configuration identifier:"

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about replacing config_id with "configuration identifier" is good for the understanding.
On the one hand, it s better English, I agree with that, but on the other hand, config_id is the name of the attribute. So it s easier to understand.

I would personally go for the name of the attribute name (with the appropriate styling like between ` or _ ) for a better understanding.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with JR here, config_id to me is straight-forward that's (to my understanding) being used in multiple places within the doc :D

@@ -70,6 +73,24 @@ The data nodes that are part of a **scenario**, **pipeline** or **task** can be
task.sales_history
```

Data nodes can be retrieved by their config_id by calling `taipy.get_entities_by_config_id()^` with their config_id.
Copy link
Member

Choose a reason for hiding this comment

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

"by their config_id" -> "using their configuration identifier"

docs/manuals/core/entities/data-node-mgt.md Outdated Show resolved Hide resolved
@@ -65,20 +65,39 @@ pipeline == pipeline_retrieved

Here the two variables `pipeline` and `pipeline_retrieved` are equal.

# Get pipeline by config id
# Get pipelines by config id

A pipeline can also be retrieved from a scenario by accessing the pipeline's config_id of the scenario.
Copy link
Member

Choose a reason for hiding this comment

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

"config_id" -> "configuration identifier"

Copy link
Member Author

Choose a reason for hiding this comment

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

as said above, I still think config_id is better as it's clear and straight to the point :D but if you think otherwise, I'm open to changing it to configuration identifier :D


# Get tasks by config id

A task can be retrieved from a scenario or a pipeline, by accessing the task config_id attribute.
Copy link
Member

Choose a reason for hiding this comment

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

The provided example does not reflect that doc line.
"accessing the task config_id attribute" means task["config_id"]

Copy link
Member

Choose a reason for hiding this comment

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

Not really.
"accessing the task config_id attribute" means task.config_id. That's what the code example shows:

scenario.predicting
pipeline.predicting

Here, predicting is the value of the attribute config_id.

Maybe we can add a sentence to make it explicit.

docs/manuals/core/entities/task-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/task-mgt.md Outdated Show resolved Hide resolved
docs/manuals/core/entities/task-mgt.md Outdated Show resolved Hide resolved
docs/relnotes.md Outdated Show resolved Hide resolved
Copy link
Member

@jrobinAV jrobinAV left a comment

Choose a reason for hiding this comment

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

Good to go!

@toan-quach toan-quach merged commit f4592eb into develop May 16, 2023
1 check passed
@toan-quach toan-quach deleted the feature/#450-get-entities-by-config-id branch May 16, 2023 02:32
arcanaxion pushed a commit to arcanaxion/taipy-doc that referenced this pull request Oct 11, 2023
…se-parent-as-property-key

feature/Avaiga#443 added warning about reserved keywords
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.

None yet

4 participants