-
Notifications
You must be signed in to change notification settings - Fork 0
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/SOF-7476 update: rewrite Standata to new specs #11
Conversation
src/py/mat3ra/standata/base.py
Outdated
|
||
|
||
class Standata: | ||
"""The Standata class associates the entity data files with categories and allows for tag-based queries. | ||
CATEGORY_SEPARATOR = "/" |
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.
Place above the first class definition
src/py/mat3ra/standata/base.py
Outdated
cf for cf in self.get_categories_as_list() if any((cf.split(CATEGORY_SEPARATOR)[-1] == t) for t in tags) | ||
] | ||
|
||
def get(self, key: str, default=None): |
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.
We shouldn't need it or it should be called get_from_categories
src/py/mat3ra/standata/base.py
Outdated
""" | ||
Initialize the StandataFilesMapByName from the input data. | ||
""" | ||
return StandataFilesMapByName(dictionary=data.get("filesMapByName", {})) |
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'd split this into 2 lines for clarity like you have it on line 179
src/py/mat3ra/standata/base.py
Outdated
""" | ||
config_data = data.get("standataConfig", {}) | ||
return StandataConfig( | ||
categories=config_data.get("categories", {}), |
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.
Same as above - let's define categories = config_data.get("categories", {})
and entities above on separate lines
|
||
|
||
class StandataBuilder: | ||
"""The Standata class associates the entity data files with categories and allows for tag-based queries. |
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 line needs to be replaced
|
||
EntityItem = TypedDict("EntityItem", {"filename": str, "categories": List[str]}) | ||
|
||
EntityConfig = TypedDict("EntityConfig", {"categories": Dict[str, List[str]], "entities": List[EntityItem]}) |
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 these should be used
|
||
for category in categories: | ||
category_dir = categories_root / category | ||
category_dir.mkdir(parents=True, exist_ok=True) | ||
linked_entity = category_dir / entity["filename"] | ||
linked_entity = category_dir / entity.filename |
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.
Does this work?
@@ -0,0 +1,31 @@ | |||
from mat3ra.standata.materials import Materials |
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.
We should combine this file and test_materials_data into test_materials_data
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.
We can make another file for test_standata_build
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 Claude generated tests for builder and CLI
try: | ||
linked_entity.symlink_to(entity_path) | ||
except PermissionError: | ||
pytest.skip("No permission to create symlinks") |
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.
Lines 101-120 and 132-149 need to be reused from above cli.py
No description provided.