Skip to content

Conversation

@nparavicini7
Copy link
Collaborator

Added a beginner/intermediate guide on the ItemGraph, remap_data(), and how to use them together, and then added a sample notebook that users can download to maintain their own organization-wide dependency graph and use it for analysis/admin workflows. Thank you @ManushiM for the section on automating it

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 8, 2025

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2025-07-08T18:36:50Z
----------------------------------------------------------------

I think it might be helpful to be explicit with the out_format argument.


@review-notebook-app
Copy link

review-notebook-app bot commented Jul 8, 2025

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2025-07-08T18:36:51Z
----------------------------------------------------------------

Line #2.    result_form = graph.get_item("41dd3cca32914cc7ae8fd3a727f44e17")

Might be helpful to call this variable result_flyr since that's what it is...rather than form?


@review-notebook-app
Copy link

review-notebook-app bot commented Jul 8, 2025

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2025-07-08T18:36:52Z
----------------------------------------------------------------

  • Is there a definitive list of JSON-based item types to link to?
  • suggestion for this sentence: At its core, it allows you to swap out data sources within your apps. Would it be more comprehensive to say "...data sources within your item's JSON structure"?
  • suggestion: "...so nothing gets messed up in application"... -> "so nothing imcompatible gets misconfigured in the applciation?"

nparavicini7 commented on 2025-07-09T22:16:03Z
----------------------------------------------------------------

Addressed 2 and 3. For 1, no, should we provide a list of the items compatible with remap_data()? There's only a subset currently. We could do an asterisk and list at the bottom or something

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 9, 2025

View / edit / reply to this conversation on ReviewNB

ManushiM commented on 2025-07-09T02:56:17Z
----------------------------------------------------------------

do we want to specify version numbers for when a feature was added?


@review-notebook-app
Copy link

review-notebook-app bot commented Jul 9, 2025

View / edit / reply to this conversation on ReviewNB

ManushiM commented on 2025-07-09T02:56:17Z
----------------------------------------------------------------

maybe replace tedious with time-consuming, to avoid the negative connotation?


Copy link
Collaborator Author

Addressed 2 and 3. For 1, no, should we provide a list of the items compatible with remap_data()? There's only a subset currently. We could do an asterisk and list at the bottom or something


View entire conversation on ReviewNB

@nparavicini7 nparavicini7 requested a review from ManushiM July 9, 2025 22:36
@nparavicini7
Copy link
Collaborator Author

Addressed all of your comments @jyaistMap @ManushiM. The one thing I didn't add was a list of the compatible JSON-based items for remap_data()- I think that would be more appropriate for the docstring of the function

@jyaistMap
Copy link
Collaborator

Addressed all of your comments @jyaistMap @ManushiM. The one thing I didn't add was a list of the compatible JSON-based items for remap_data()- I think that would be more appropriate for the docstring of the function

Good idea. Sounds appropriate.

Copy link
Collaborator

@jyaistMap jyaistMap left a comment

Choose a reason for hiding this comment

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

  • submitted #2319 for formatting and spelling - after that lgtm

Update hyperlink formats and misspellings
@nparavicini7 nparavicini7 requested a review from jyaistMap July 10, 2025 18:34
@nparavicini7 nparavicini7 merged commit 179a099 into master Jul 10, 2025
3 checks passed
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