feat(system): add config submodule#56
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds a new system.config scripting submodule backed by a PyResource wrapper, plus matching type stubs and a minor documentation link update. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
configfunctions andPyResource.__init__currently useprintfor debugging, which will be noisy for consumers of this API; consider removing these or switching to a configurable logging mechanism before this module is relied on in production. - Several docstrings appear to be copy/pasted or contain small inaccuracies (e.g.,
replacedescribes copying and mentionsnewName/newCollection,deletehasforce: f true), so tightening these up to reflect the actual semantics will make the API much clearer to users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `config` functions and `PyResource.__init__` currently use `print` for debugging, which will be noisy for consumers of this API; consider removing these or switching to a configurable logging mechanism before this module is relied on in production.
- Several docstrings appear to be copy/pasted or contain small inaccuracies (e.g., `replace` describes copying and mentions `newName`/`newCollection`, `delete` has `force: f true`), so tightening these up to reflect the actual semantics will make the API much clearer to users.
## Individual Comments
### Comment 1
<location path="src/system/config.py" line_range="152-153" />
<code_context>
+ collection: The collection containing the resource. If omitted,
+ uses the active definition. Optional.
+ signature: The hex-encoded signature of the resource. Optional.
+ force: f true, deletes the resource even if other resources
+ reference it. If omitted, default is false. Optional.
+ actor: A string identifying the actor performing the operation.
</code_context>
<issue_to_address>
**nitpick (typo):** Fix the typo and capitalization in the `force` parameter description.
Update the `force` docstring to something like: `If true, deletes the resource even if other resources reference it. If omitted, the default is False.` This fixes the typo, clarifies behavior, and uses standard boolean capitalization.
```suggestion
force: If true, deletes the resource even if other resources
reference it. If omitted, the default is False. Optional.
```
</issue_to_address>
### Comment 2
<location path="src/system/config.py" line_range="314-41" />
<code_context>
+ return PyResource()
+
+
+def replace(
+ moduleId, # type: Union[str, unicode]
+ typeId, # type: Union[str, unicode]
+ name=None, # type: Union[str, unicode, None]
+ collection=None, # type: Union[str, unicode, None]
+ signature=None, # type: Union[str, unicode, None]
+ config=None, # type: Optional[Dict[Union[str, unicode], Any]]
+ backupConfig=None, # type: Optional[Dict[Union[str, unicode], Any]]
+ files=None, # type: Optional[Dict[Union[str, unicode], Any]]
+ description=None, # type: Union[str, unicode, None]
+ enabled=None, # type: Optional[bool]
+ attributes=None, # type: Optional[Dict[Union[str, unicode], Any]]
+ actor=None, # type: Union[str, unicode, None]
+):
+ # type: (...) -> PyResource
+ """Copies a resource to a new name and/or collection.
+
+ When using this function, either the newName or newCollection
+ parameter must be defined.
+
</code_context>
<issue_to_address>
**issue:** Update the `replace` docstring so it accurately describes a replace operation and matches the parameters.
The current docstring still describes a copy operation and references `newName`/`newCollection`, which aren’t parameters of this function. Please update it to describe the replace behavior and remove references to non-existent arguments so callers aren’t misled.
</issue_to_address>
### Comment 3
<location path="src/system/config.py" line_range="183-49" />
<code_context>
+ return []
+
+
+def getResource(
+ moduleId, # type: Union[str, unicode]
+ typeId, # type: Union[str, unicode]
+ name, # type: Union[str, unicode]
+ collection, # type: Union[str, unicode]
+):
+ # type: (...) -> PyResource
+ """Returns a resource from the Gateway.
+
+ Args:
+ moduleId: The module ID portion of the resource type identifier.
+ typeId: The type ID portion of the resource type identifier.
+ name: The name of the resource. Required for named resources,
+ but must be omitted for singleton resources.
+ collection: The collection containing the resource. If omitted,
+ uses the active definition.
+
</code_context>
<issue_to_address>
**question:** Consider reconciling the `getResource` docstring with the function signature regarding the `collection` argument.
The docstring says `collection` may be omitted, but the function requires it as a positional argument with no default. Either make `collection` optional with a default of `None` (and update the type hint) or update the docstring to state that `collection` is required.
</issue_to_address>
### Comment 4
<location path="src/system/config.py" line_range="116-125" />
<code_context>
+ your newly created Gateway resource, which can also be read as
+ plain Python properties.
+ """
+ print(
+ moduleId,
+ typeId,
+ name,
+ collection,
+ config,
+ backupConfig,
+ files,
+ description,
+ enabled,
+ attributes,
+ actor,
+ )
+ return PyResource()
</code_context>
<issue_to_address>
**suggestion:** Avoid unconditional `print` statements in library functions.
In `create` (and similar functions in this module), printing all arguments will generate unexpected console output for library consumers. Please either switch to a proper logging mechanism or remove these prints if they were only for debugging/stub behavior.
Suggested implementation:
```python
Returns:
A PyResource containing the specified parameter attributes of
your newly created Gateway resource, which can also be read as
plain Python properties.
"""
def create(
```
```python
return PyResource()
```
If similar debugging `print(...)` calls were added to other functions in this module (e.g., `update`, `delete`, etc.), they should be removed or replaced with calls to a configured logger (`logging.getLogger(__name__).debug(...)`) to avoid unexpected console output for library consumers.
</issue_to_address>
### Comment 5
<location path="src/system/config.py" line_range="227-229" />
<code_context>
+ return [PyResource()]
+
+
+def getResourceTypes():
+ # type: () -> List[Any]
+ """Returns a list of all registered resource types.
+
+ Returns:
</code_context>
<issue_to_address>
**suggestion:** Consider tightening the return type of `getResourceTypes` to reflect the documented tuple structure.
The docstring promises `a list of tuples containing all the currently registered resource types`, but the annotation is `List[Any]`. If that tuple structure is stable, please update the type to something like `List[Tuple[...]]` and keep the `.pyi` stub in sync so callers get accurate type information.
Suggested implementation:
```python
def getResourceTypes():
# type: () -> List[Tuple[str, str]]
"""Returns a list of all registered resource types.
Returns:
```
` since it’s not shown).
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="src/system/config.py">
<<<<<<< SEARCH
def getResourceTypes():
# type: () -> List[Any]
"""Returns a list of all registered resource types.
Returns:
=======
def getResourceTypes():
# type: () -> List[Tuple[str, str]]
"""Returns a list of all registered resource types.
Returns:
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. Ensure `Tuple` is imported from `typing` at the top of `src/system/config.py`, for example:
<<<<<<< SEARCH
from typing import Any, List
=======
from typing import Any, List, Tuple
>>>>>>> REPLACE
Adjust the SEARCH line to match the existing import(s) in your file.
2. Update the corresponding stub file (likely `src/system/config.pyi` or similar) so that the `getResourceTypes` signature uses the same tightened type:
`def getResourceTypes() -> List[Tuple[str, str]]: ...`
3. If the tuple structure contains more or different fields than two `str` values, refine the `Tuple[...]` contents accordingly in both `.py` and `.pyi` files (e.g. `Tuple[str, str, int]` or a `TypedDict`/`NamedTuple` if appropriate).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+152
to
+153
| force: f true, deletes the resource even if other resources | ||
| reference it. If omitted, default is false. Optional. |
Contributor
There was a problem hiding this comment.
nitpick (typo): Fix the typo and capitalization in the force parameter description.
Update the force docstring to something like: If true, deletes the resource even if other resources reference it. If omitted, the default is False. This fixes the typo, clarifies behavior, and uses standard boolean capitalization.
Suggested change
| force: f true, deletes the resource even if other resources | |
| reference it. If omitted, default is false. Optional. | |
| force: If true, deletes the resource even if other resources | |
| reference it. If omitted, the default is False. Optional. |
| typeId: The type ID portion of the resource type identifier. | ||
| name: The name of the resource. Required for named resources, | ||
| but must be omitted for singleton resources. Optional. | ||
| collection: The collection containing the resource. If omitted, |
Contributor
There was a problem hiding this comment.
question: Consider reconciling the getResource docstring with the function signature regarding the collection argument.
The docstring says collection may be omitted, but the function requires it as a positional argument with no default. Either make collection optional with a default of None (and update the type hint) or update the docstring to state that collection is required.
Comment on lines
+116
to
+125
| print( | ||
| moduleId, | ||
| typeId, | ||
| name, | ||
| collection, | ||
| config, | ||
| backupConfig, | ||
| files, | ||
| description, | ||
| enabled, |
Contributor
There was a problem hiding this comment.
suggestion: Avoid unconditional print statements in library functions.
In create (and similar functions in this module), printing all arguments will generate unexpected console output for library consumers. Please either switch to a proper logging mechanism or remove these prints if they were only for debugging/stub behavior.
Suggested implementation:
Returns:
A PyResource containing the specified parameter attributes of
your newly created Gateway resource, which can also be read as
plain Python properties.
"""
def create( return PyResource()If similar debugging print(...) calls were added to other functions in this module (e.g., update, delete, etc.), they should be removed or replaced with calls to a configured logger (logging.getLogger(__name__).debug(...)) to avoid unexpected console output for library consumers.
Comment on lines
+227
to
+229
| def getResourceTypes(): | ||
| # type: () -> List[Any] | ||
| """Returns a list of all registered resource types. |
Contributor
There was a problem hiding this comment.
suggestion: Consider tightening the return type of getResourceTypes to reflect the documented tuple structure.
The docstring promises a list of tuples containing all the currently registered resource types, but the annotation is List[Any]. If that tuple structure is stable, please update the type to something like List[Tuple[...]] and keep the .pyi stub in sync so callers get accurate type information.
Suggested implementation:
def getResourceTypes():
# type: () -> List[Tuple[str, str]]
"""Returns a list of all registered resource types.
Returns:` since it’s not shown).
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="src/system/config.py">
<<<<<<< SEARCH
def getResourceTypes():
# type: () -> List[Any]
"""Returns a list of all registered resource types.
Returns:
=======
def getResourceTypes():
# type: () -> List[Tuple[str, str]]
"""Returns a list of all registered resource types.
Returns:
REPLACE
</file_operation>
</file_operations>
<additional_changes>
-
Ensure
Tupleis imported fromtypingat the top ofsrc/system/config.py, for example:<<<<<<< SEARCH
from typing import Any, Listfrom typing import Any, List, Tuple
REPLACE
Adjust the SEARCH line to match the existing import(s) in your file.
-
Update the corresponding stub file (likely
src/system/config.pyior similar) so that thegetResourceTypessignature uses the same tightened type:
def getResourceTypes() -> List[Tuple[str, str]]: ... -
If the tuple structure contains more or different fields than two
strvalues, refine theTuple[...]contents accordingly in both.pyand.pyifiles (e.g.Tuple[str, str, int]or aTypedDict/NamedTupleif appropriate).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #54
Summary by Sourcery
Introduce a system.config API for managing Gateway resources, backed by a PyResource abstraction, and update documentation to reference the latest Jython standalone version.
New Features:
Enhancements:
Documentation: