Skip to content

Conversation

@saurbhc
Copy link
Member

@saurbhc saurbhc commented Dec 5, 2023

Problem

Feature request: need utility functions for parsing metadata.json to import Properties when importing Annotations.

Solution

Parse metadata.json to create list of newly introduced Property class.

Changelog

Parsing metadata.json for Properties Import

@linear
Copy link

linear bot commented Dec 5, 2023

@saurbhc saurbhc marked this pull request as ready for review December 6, 2023 11:59
@saurbhc saurbhc requested a review from Nathanjp91 December 6, 2023 11:59
with open(path) as f:
manifest = json.load(f)

return manifest
Copy link
Contributor

Choose a reason for hiding this comment

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

can we parse_manifest(manifest) here as well so we're returning the list[property] rather than raw dict

Copy link
Member Author

@saurbhc saurbhc Dec 6, 2023

Choose a reason for hiding this comment

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

the reason why I put both parsing functions in different files:

  • parse_manifest in path_utils.py
  • parse_properties in datatypes.py
    is because it needs the Property class to be imported from datatypes.py file - and when imported in path_utils.py, it creates a cyclic dependency on both files.

Other solution would be to move the Property class to path_utils.py? Thoughts?

Copy link
Contributor

@Nathanjp91 Nathanjp91 left a comment

Choose a reason for hiding this comment

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

Some things to change.

@saurbhc saurbhc requested a review from Nathanjp91 December 6, 2023 16:34
@saurbhc saurbhc changed the title [PY-619] manifest.json parser utilities [PY-619] metadata.json parser utilities Dec 6, 2023
@saurbhc saurbhc self-assigned this Dec 6, 2023
@saurbhc saurbhc added the enhancement New feature or request label Dec 6, 2023
Copy link
Contributor

@Nathanjp91 Nathanjp91 left a comment

Choose a reason for hiding this comment

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

Looks good.

@saurbhc saurbhc merged commit c8a2d35 into master Dec 12, 2023
@saurbhc saurbhc deleted the PY-619 branch December 12, 2023 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants