parse dependencies and add to header as #r entries#10
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the --dev flag functionality by parsing package dependencies from pyproject.toml or requirements files and adding them as #r entries in Grasshopper component headers, enabling easier development setups.
Key Changes:
- Added dependency parsing logic from
pyproject.toml(static and dynamic) and requirements files - Modified the
update_gh_headerfunction to include parsed dependencies when--devflag is used - Updated CHANGELOG.md to document the new feature
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/compas_invocations2/grasshopper.py | Added three new helper functions (_sanitize_dependency, _get_deps_from_requirements, _get_dependencies) to parse and sanitize dependencies, and integrated them into the update_gh_header function to add dependencies to the header when --dev is enabled |
| CHANGELOG.md | Added entry documenting the new --dev flag enhancement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new_header.append(f"# env: {env.strip()}\n") | ||
| if dev: | ||
| dependencies = _get_dependencies(ctx.base_folder) | ||
| new_header.append(f"# r: {', '.join(dependencies)}\n") |
There was a problem hiding this comment.
If dependencies is an empty list, this will add an empty # r: line to the header, which might not be the intended behavior. Consider checking if the list is non-empty before adding it:
if dependencies:
new_header.append(f"# r: {', '.join(dependencies)}\n")| new_header.append(f"# r: {', '.join(dependencies)}\n") | |
| if dependencies: | |
| new_header.append(f"# r: {', '.join(dependencies)}\n") |
| def _sanitize_dependency(dep: str) -> str: | ||
| # Remove upper bound constraints (e.g., ", <3" or ",<3") as Rhino doesn't support them | ||
| sanitized = re.split(r"\s*,\s*[<>=]", dep)[0] | ||
| sanitized = sanitized.split("#")[0].strip() # Remove inline comments | ||
| return sanitized | ||
|
|
||
|
|
||
| def _get_deps_from_requirements(req_filepath: str) -> str: | ||
| with open(req_filepath, "r") as req_file: | ||
| dependencies = [] | ||
| for line in req_file: | ||
| line = line.strip() | ||
| if line and not line.startswith("#"): | ||
| dependencies.append(_sanitize_dependency(line)) | ||
| return dependencies | ||
|
|
||
|
|
||
| def _get_dependencies(base_folder: str) -> str: | ||
| toml_filepath = os.path.join(base_folder, "pyproject.toml") | ||
| with open(toml_filepath, "r") as f: | ||
| toml = tomlkit.load(f) | ||
|
|
||
| dependencies = toml.get("project").get("dependencies") | ||
| if dependencies: | ||
| return [_sanitize_dependency(dep) for dep in dependencies] | ||
|
|
||
| dynamic_deps = toml.get("tool").get("setuptools").get("dynamic").get("dependencies") | ||
| if dynamic_deps and "file" in dynamic_deps: | ||
| req_filepath = os.path.join(base_folder, dynamic_deps["file"]) | ||
| return _get_deps_from_requirements(req_filepath) | ||
|
|
||
| return [] |
There was a problem hiding this comment.
The new functions _sanitize_dependency, _get_deps_from_requirements, and _get_dependencies lack docstrings explaining their purpose, parameters, and return values. This makes it harder for other developers to understand their behavior. Consider adding docstrings following the project's documentation standards.
| return sanitized | ||
|
|
||
|
|
||
| def _get_deps_from_requirements(req_filepath: str) -> str: |
There was a problem hiding this comment.
The return type annotation is incorrect. The function returns a list of strings, not a str. This should be -> list[str] or -> List[str] (with from typing import List).
| def _get_deps_from_requirements(req_filepath: str) -> str: | |
| def _get_deps_from_requirements(req_filepath: str) -> list[str]: |
| return dependencies | ||
|
|
||
|
|
||
| def _get_dependencies(base_folder: str) -> str: |
There was a problem hiding this comment.
The return type annotation is incorrect. The function returns a list of strings, not a str. This should be -> list[str] or -> List[str] (with from typing import List).
| def _get_dependencies(base_folder: str) -> str: | |
| def _get_dependencies(base_folder: str) -> list[str]: |
| dependencies = toml.get("project").get("dependencies") | ||
| if dependencies: | ||
| return [_sanitize_dependency(dep) for dep in dependencies] | ||
|
|
||
| dynamic_deps = toml.get("tool").get("setuptools").get("dynamic").get("dependencies") |
There was a problem hiding this comment.
This code will raise an AttributeError if any of the nested keys don't exist. The .get() method returns None by default, and calling .get() on None will fail. Consider adding proper null checks or using a try-except block. For example:
dependencies = toml.get("project", {}).get("dependencies")Or use a try-except to handle the case where the structure doesn't match expectations.
| dependencies = toml.get("project").get("dependencies") | |
| if dependencies: | |
| return [_sanitize_dependency(dep) for dep in dependencies] | |
| dynamic_deps = toml.get("tool").get("setuptools").get("dynamic").get("dependencies") | |
| dependencies = toml.get("project", {}).get("dependencies") | |
| if dependencies: | |
| return [_sanitize_dependency(dep) for dep in dependencies] | |
| dynamic_deps = toml.get("tool", {}).get("setuptools", {}).get("dynamic", {}).get("dependencies") |
| if dependencies: | ||
| return [_sanitize_dependency(dep) for dep in dependencies] | ||
|
|
||
| dynamic_deps = toml.get("tool").get("setuptools").get("dynamic").get("dependencies") |
There was a problem hiding this comment.
This code will raise an AttributeError if any of the nested keys don't exist. The chain of .get() calls returns None if any key is missing, and calling .get() on None will fail. Consider adding proper null checks or using a try-except block. For example:
dynamic_deps = toml.get("tool", {}).get("setuptools", {}).get("dynamic", {}).get("dependencies")| dynamic_deps = toml.get("tool").get("setuptools").get("dynamic").get("dependencies") | |
| dynamic_deps = ( | |
| toml.get("tool", {}) | |
| .get("setuptools", {}) | |
| .get("dynamic", {}) | |
| .get("dependencies") | |
| ) |
gonzalocasas
left a comment
gonzalocasas
left a comment
There was a problem hiding this comment.
Code looks good to me, but I haven't really understood what's the use-case this solves, in dev mode, I would I assume the components don't really need any # r: and one would have a custom python component running first in the GH file that has the # r: with all the locally relevant stuff, but I am surely missing something!
Also: Rhino doesn't support upper bounds? What sort of bizarre limit is that? Is it a problem with encoding because they are passing the <3 as piping from something and needs to be escaped or the like? Or is it an actual limit in their implementation?
yes indead the comma is not escaped and this is what the resulting pip command looks like: |
|
omg.. haha.. have we reported it to them? If so, please add a link to the forum post or similar so that eventually we read this again, and we understand why we did this. Maybe also add a |
This makes it easier to create isolated dev environments for plugin development. so for example which is all one needs in order to get this component to run, backed by an editable version of the library. this is part of a workflow that looks like this:
|
|
Ahh makes sense now! Thanks |
opened https://discourse.mcneel.com/t/python-dependencies-with-ordered-comparison-syntax/212304! |
--devflag now also adds the package requirements to the header as#rentries.This allows easy dev setups for development of GH components.
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.