Generate API wrapper from openapi specs#24
Conversation
…d improve file structure
WalkthroughAdds a specs-to-Python codegen CLI and parser framework (parsers, spec models, and code generators) that emit an API library scaffold (requests/responses modules and api.py). Also updates repository tooling (removes Makefile fmt/test targets, adds Justfile), adjusts tests/README examples to plain type annotations, updates .gitignore, pytest verbosity, and CI to use just. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as from_specs CLI
participant FS as File System
participant Specs as Specs Parser
participant Gen as Codegen (objects)
participant Utils as Utilities
Dev->>CLI: run `-s .specs.json -n generated_api --prefix /api/v4`
CLI->>FS: read specs JSON
CLI->>Specs: parse JSON -> Specs instance
CLI->>Utils: manage_directories(output/, output/responses/, output/requests/)
loop per schema
CLI->>Gen: object_file_content(schema_name, type)
Gen-->>CLI: generated source text
CLI->>FS: write `.../responses|requests/<snake_name>.py`
end
CLI->>Specs: method_strings(prefix)
Specs-->>CLI: list of method definitions
CLI->>FS: write `.../api.py` and `__init__.py`
CLI-->>Dev: exit (scaffold created)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)api_lib/from_specs/objects/components.py (2)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeandemeusy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
64-66: Wheel packaging misconfiguration: absolute package path likely breaks builds.
packages = ["/api_lib"]points to an absolute path, which will fail to include your package in wheels. It should be relative:"api_lib".Apply this diff:
[tool.hatch.build.targets.wheel] -packages = ["/api_lib"] +packages = ["api_lib"]Optionally, consider using
only-include = ["api_lib"]instead ofpackagesfor clarity when using Hatch.README.md (1)
46-49: Variable-name mismatch in the example breaks copy/paste.You instantiate
api = GithubAPI(...)but then callawait gh_api.user(). Align the variable name.Apply one of these:
-api = GithubAPI("https://api.github.com", Bearer(env_var="GITHUB_TOKEN")) - -user = await gh_api.user() +gh_api = GithubAPI("https://api.github.com", Bearer(env_var="GITHUB_TOKEN")) +user = await gh_api.user()or
-api = GithubAPI("https://api.github.com", Bearer(env_var="GITHUB_TOKEN")) - -user = await gh_api.user() +api = GithubAPI("https://api.github.com", Bearer(env_var="GITHUB_TOKEN")) +user = await api.user()
🧹 Nitpick comments (19)
.gitignore (1)
32-35: Ignore patterns look good; minor nits on duplication and scope.
- Good: Adding
.DS_Store,.generated.*, and*.specs.jsonaligns with the codegen workflow.- Nits:
*.logappears twice (Line 14 and Line 22). Keep one to avoid drift.- Verify whether you truly want to ignore all
*.specs.jsoninputs. If specs are part of the reproducible source (common for OpenAPI), consider tracking them or keeping a canonical example underdocs/ortest/fixtures/.Would you like me to open a follow-up to de-dup the pattern and/or move a sample spec into
docs/?pyproject.toml (1)
60-60: Changing pytest log level to CRITICAL reduces signal during failures.If a test fails intermittently, you might lose valuable context. Consider
ERRORin CI andINFOlocally viaPYTEST_ADDOPTSor an env-guarded override.If desired, I can wire a small
conftest.pythat bumpslog_cli_leveltoINFOwhenCIenv var is not set.README.md (1)
21-49: Optional: make the async usage runnable as-is.Top-level
awaitis fine in a REPL, but many users will paste into a file. Consider wrapping inasync def main()+asyncio.run(main()).Would you like me to propose a short, runnable snippet with
asyncio.run?test/config/request.py (2)
8-9: Makingper_pageandpagerequired may break existing callers.Previously these had defaults (via
APIfield()), now callers must always pass them. If this is intentional, ignore. If you want backward compatibility and omission to exclude the param from queries, declare them optional withNonedefault.Here’s a compatible alternative:
- per_page: str - page: str + per_page: Optional[str] = None + page: Optional[str] = NoneThis works with
RequestData.as_query_parameters, which omitsNonevalues.
14-16: Similarly,fieldbecoming required changes the constructor API.If the goal is to require it, all good. Otherwise, mirror prior behavior with an explicit default.
- field: str + field: Optional[str] = NoneI can update/augment tests if you want to preserve prior call sites.
api_lib/from_specs/objects/parser.py (2)
4-4: Decorator alias is fine; consider slots/kw-only for generated objects.
ParserObject = dataclass(init=False)works as a decorator to keepParser.__init__in control. For perf and immutability of layout, consider:
slots=Trueto reduce memory footprint for many generated objects.kw_only=Trueto make accidental positional instantiation obvious.If you adopt
slots=True, adjust__repr__to avoidvars(self)(see another comment).Proposed tweak:
-ParserObject = dataclass(init=False) +ParserObject = dataclass(init=False, slots=True, kw_only=True)
36-38:__repr__based onvars(self)breaks withslots=Trueand may render ambiguous values.Prefer rendering via dataclass fields and
!rto disambiguate strings and nested objects. This also stays compatible if you adoptslots=True.- def __repr__(self): - key_pair_string: str = ", ".join([f"{key}={value}" for key, value in vars(self).items()]) - return f"{self.__class__.__name__}({key_pair_string})" + def __repr__(self): + contents = ", ".join(f"{f.name}={getattr(self, f.name)!r}" for f in fields(self)) + return f"{self.__class__.__name__}({contents})"test/config/response.py (1)
38-39: Nested object fields: consider Optional types if payloads omit sub-objectsuser: User and repository: Repository will attempt to construct sub-objects whenever present; if the payload may omit these keys, annotating as User | None / Repository | None would more accurately reflect reality and avoid surprising None assignments against non-Optional types. Up to you based on spec guarantees.
api_lib/from_specs/objects/tags.py (1)
4-7: Align with OpenAPI: make description optional; consider externalDocs supportIn OAS, Tag.description is optional, and tags may include externalDocs. Recommend:
- Make description: str | None = None
- Optionally add externalDocs (a small object with url and description) later if needed by the generator.
Apply this minimal change:
@ParserObject class Tags(Parser): - name: str - description: str + name: str + description: str | None = Noneapi_lib/from_specs/lib.py (2)
4-7: snakecase: strengthen for acronyms, digits, hyphens, and whitespaceCurrent logic splits only on uppercase transitions and misses hyphens/spaces; "HTTPServer v2" becomes "h_t_t_p_server v2". Use regex to handle acronyms and non-alphanumerics.
Apply:
-from pathlib import Path +from pathlib import Path +import re @@ -def snakecase(text: str) -> str: - if text.islower() or not text: - return text - return text[0].lower() + "".join("_" + x.lower() if x.isupper() else x for x in text[1:]) +def snakecase(text: str) -> str: + """Convert CamelCase, kebab-case, and mixed strings to snake_case.""" + if not text: + return text + s = re.sub(r"([A-Z]+)([A-Z][a-z])", r"\1_\2", text) # HTTPServer -> HTTP_Server + s = re.sub(r"([a-z0-9])([A-Z])", r"\1_\2", s) # userID -> user_ID + s = re.sub(r"[^0-9A-Za-z]+", "_", s) # spaces/hyphens -> _ + return s.strip("_").lower()Optionally add unit cases: ["HTTPServer", "UserID42", "user-id", "user id", "already_snake"].
10-20: manage_directories: future-proof recursive clean-upWhile the current generator only writes
.pyfiles directly under the two top-level folders (so you won’t hit the subdirectory bug today), it’s safer to hardenmanage_directoriesagainst any nested content. The refactor below:
- always ensures the target directory exists
- walks its contents recursively, so no orphaned subfolders cause
unlink()orrmdir()to fail- preserves the directory itself (rather than deleting+recreating it)
- adds a return‐type annotation for clarity
Apply the following diff in
api_lib/from_specs/lib.py(lines 10–20):from pathlib import Path +import shutil -def manage_directories(*directories: Path): - """Create directories if they do not exist, or clear them if they do.""" +def manage_directories(*directories: Path) -> tuple[Path, ...]: + """Ensure each directory exists and is empty. + Recursively removes all files, symlinks, and subdirectories.""" for directory in directories: - if directory.exists(): - for item in directory.iterdir(): - item.unlink() - directory.rmdir() - - directory.mkdir(parents=True, exist_ok=True) + # make sure the directory is present + directory.mkdir(parents=True, exist_ok=True) + # clear every entry, recursing into subdirectories + for item in directory.iterdir(): + if item.is_file() or item.is_symlink(): + item.unlink() + elif item.is_dir(): + shutil.rmtree(item) + + return directoriesThis change ensures no future nested folders can break your clean-up step while keeping the API and return value unchanged.
justfile (1)
14-16: Parametrize specs inputs/outputs to improve ergonomicsAllow overriding the specs path and output directory via variables. This keeps default behavior intact and makes the target reusable.
-specs: - @uv run -m api_lib.from_specs -s .specs.json -n .generated.hoprd +specs SPEC?=.specs.json OUT?=.generated.hoprd: + @uv run -m api_lib.from_specs -s {{SPEC}} -n {{OUT}}Optional: add a clean target for OUT via rm -rf.
api_lib/from_specs/__main__.py (3)
12-19: Consider makingwrite_content_to_filemore robust.The function should validate that the parent directory exists before attempting to write, and the file opening could benefit from explicit encoding specification.
def write_content_to_file(file: Path, content: Optional[str] = None): """Writes the content to a file.""" if not content: content = "" - with file.open("w") as f: + file.parent.mkdir(parents=True, exist_ok=True) + with file.open("w", encoding="utf-8") as f: f.write(content)
50-51: Simplify the conditional logic for type and folder assignment.The type determination and folder assignment can be streamlined using a single conditional expression.
- type: ObjectType = ObjectType.RESPONSE if cls_name in specifications.response_objects else ObjectType.REQUEST - folder: Path = resp_folder if type == ObjectType.RESPONSE else req_folder + type = ObjectType.RESPONSE if cls_name in specifications.response_objects else ObjectType.REQUEST + folder = resp_folder if type == ObjectType.RESPONSE else req_folder
62-73: Use textwrap.dedent for better readability of multi-line strings.The current string formatting with manual indentation can be improved using Python's
textwrap.dedentfor cleaner code.Add the import at the top of the file:
from textwrap import dedentThen refactor the content generation:
- # Create the api.py file - main_file_content = \ -f""" -from api_lib import ApiLib -from api_lib.method import Method - -from . import requests, responses - - -class {specifications.info.class_title}(ApiLib): -{'\n'.join([f"\t{line}" for line in specifications.method_strings])} -""".strip() + # Create the api.py file + method_lines = '\n'.join([f" {line}" for line in specifications.method_strings]) + main_file_content = dedent(f""" + from api_lib import ApiLib + from api_lib.method import Method + + from . import requests, responses + + + class {specifications.info.class_title}(ApiLib): + {method_lines} + """).strip()api_lib/from_specs/objects/specs.py (2)
30-32: Use a more Pythonic approach to iterate over dataclass fields.Instead of using
vars()which relies on implementation details, consider using the dataclass fields API or explicitly listing the HTTP methods.for key, value in self.paths.items(): - for method in vars(value): - query = getattr(value, method) + for method in ["get", "post", "delete", "put", "patch"]: + query = getattr(value, method, None)
41-46: Simplify the nested dictionary navigation logic.The current approach with a loop and conditional checks can be simplified using a try-except block or a helper function for safe dictionary navigation.
- path = ["application/json", "schema", "$ref"] - - ref = response.content - for part in path: - ref = ref.get(part, None) if ref else None + try: + ref = response.content["application/json"]["schema"]["$ref"] + except (KeyError, TypeError): + ref = Noneapi_lib/from_specs/objects/paths.py (1)
56-59: Improve method name sanitization for edge cases.The current sanitization might not handle all special characters that could appear in operationId. Consider using a more comprehensive approach.
@property def method_name(self) -> str: """Returns the method name in a string format.""" - return self.operationId.replace("-", "_").replace(" ", "_").lower() + import re + # Replace non-alphanumeric characters with underscores + name = re.sub(r'[^a-zA-Z0-9_]', '_', self.operationId) + # Remove leading/trailing underscores and collapse multiple underscores + name = re.sub(r'_+', '_', name).strip('_') + return name.lower()api_lib/from_specs/objects/components.py (1)
104-122: Consider using textwrap for description formatting.Python's
textwrapmodule provides a more robust solution for wrapping text that handles edge cases better.+ import textwrap + description_str = self.description.replace("\n", " ") - - descriptions_lines = [] - description_words = description_str.split(" ") - - if len(description_words) == 0: - description_str = "No description" - descriptions_lines = [description_str] - else: - index = 0 - while len(description_words) > 0: - descriptions_lines.append("") - while (len(descriptions_lines[index]) + len(description_words[0]) + 1) <= 95: - descriptions_lines[index] += f" {description_words.pop(0)}" - if len(description_words) == 0: - break - descriptions_lines[index] = descriptions_lines[index].strip() - index += 1 + + if not description_str.strip(): + descriptions_lines = ["No description"] + else: + descriptions_lines = textwrap.wrap(description_str, width=95)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.gitignore(1 hunks)Makefile(0 hunks)README.md(1 hunks)api_lib/from_specs/__main__.py(1 hunks)api_lib/from_specs/lib.py(1 hunks)api_lib/from_specs/objects/components.py(1 hunks)api_lib/from_specs/objects/infos.py(1 hunks)api_lib/from_specs/objects/parser.py(1 hunks)api_lib/from_specs/objects/paths.py(1 hunks)api_lib/from_specs/objects/specs.py(1 hunks)api_lib/from_specs/objects/tags.py(1 hunks)justfile(1 hunks)pyproject.toml(2 hunks)test/config/request.py(1 hunks)test/config/response.py(1 hunks)
💤 Files with no reviewable changes (1)
- Makefile
🧰 Additional context used
🧬 Code graph analysis (5)
api_lib/from_specs/objects/tags.py (1)
api_lib/from_specs/objects/parser.py (1)
Parser(7-38)
api_lib/from_specs/objects/paths.py (1)
api_lib/from_specs/objects/parser.py (1)
Parser(7-38)
test/config/request.py (1)
api_lib/objects/request.py (1)
RequestData(28-69)
api_lib/from_specs/objects/specs.py (3)
api_lib/from_specs/objects/parser.py (1)
Parser(7-38)api_lib/from_specs/objects/paths.py (2)
Methods(76-91)to_methods_strings(83-91)api_lib/from_specs/objects/tags.py (1)
Tags(5-7)
test/config/response.py (2)
api_lib/objects/response.py (4)
APIfield(10-26)JsonResponse(77-109)MetricResponse(112-151)APImetric(29-42)test/test_objects/test_response.py (1)
test_api_field(17-20)
🔇 Additional comments (8)
pyproject.toml (1)
10-13: Adding Click dependency is appropriate for the new CLI.This aligns with the new
from_specsentry point. No concerns here.README.md (1)
33-35: Nice simplification: plain types instead of APIfield wrappers.This mirrors the tests and the new codegen direction. Looks good.
test/config/response.py (2)
49-49: Metrics without labels: behavior preservedmetric_one: float and metric_three: int will aggregate unlabeled values across matching lines as before. Keep APImetric for labeled cases (metric_two/four) as you’ve done.
Also applies to: 51-51
12-12: No test dependencies on removed APIfield metadata
Verified via ripgrep that the migrated fields—Repository.name(line 12),User.login/name/disk_usage(lines 18–20), andResponseClass.field(line 26)—now rely solely on their default JSON keys and that no callers or tests reference the oldAPIfield/APImetricmetadata. All existing tests pass unchanged. Approving these changes.api_lib/from_specs/objects/specs.py (1)
16-24: LGTM! Clean implementation of method string aggregation.The property efficiently collects method strings from all paths.
api_lib/from_specs/objects/paths.py (3)
83-91: LGTM! Clean iteration over HTTP methods.The implementation correctly iterates through all HTTP methods and aggregates their string representations.
37-37: Handle edge case when ref is None before string operations.The code performs string operations on
refbut includes a conditional check for None after the operation. This could cause an AttributeError if ref is None.- ref: str = ref.split("/")[-1].rstrip("Response") if ref else "NO_REF" + if ref and ref != "NO_REF": + ref = ref.split("/")[-1].rstrip("Response") + else: + ref = "NO_REF"Likely an incorrect or invalid review comment.
71-71: No change needed:ret_typeis inserted as a Python type, not a string literalThe generator builds
ret_typeas a raw code snippet (e.g.responses.Fooorlist[responses.Foo]), without surrounding quotes, sotry_reqalways receives a Callable or GenericAlias rather than a string. The implementation inapi_lib/api_lib.pythen correctly usesget_origin/get_argsand calls the provided type or its element type, as intended.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/lint_and_test.yaml (4)
26-29: Solid addition: Just installer; consider pinning by commit SHA for supply-chain hardeningUsing extractions/setup-just@v3 with an explicit just-version is good. For stronger integrity, pin the action to a commit SHA (and optionally enable GitHub’s
actions: read-only permissions at the workflow/job level). A brief smoke check step (e.g.,just --version) after installation can also catch install issues early.Would you like me to propose a follow-up step to verify the installation and an example of pinning this action by SHA?
33-33: Fix trailing whitespace flagged by yamllint (line 33)There’s trailing spaces on the blank line after the
uv syncstep. Remove them to satisfy yamllint.Apply this diff:
- +
40-41: Trim trailing whitespace (line 41) and keep the job conciseYamllint reports trailing spaces after the pytest step. Delete the trailing spaces (and the superfluous blank line if not needed).
Apply this diff:
- name: Test with pytest working-directory: . - run: just test - + run: just test
36-36: Use a check-only recipe in CI so formatting issues break the buildThe current
fmttask in./justfileauto-formats code (viablackandruff --fix), so runningjust fmtin CI will always succeed—even when files are unformatted. To ensure the lint step fails on unformatted code:– In
./justfile, add afmt-checkrecipe alongsidefmt:fmt-check: @uvx black --check api_lib test @uvx ruff check api_lib test --output-format=github– In
.github/workflows/lint_and_test.yaml(around line 36), change the lint step:- run: just fmt + run: just fmt-checkWith this, any formatting or lint violations will cause the CI job to fail, while
just fmtcan still be used locally to auto-fix issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/lint_and_test.yaml(1 hunks)api_lib/from_specs/__main__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api_lib/from_specs/main.py
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/lint_and_test.yaml
[error] 33-33: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api_lib/from_specs/objects/paths.py (1)
27-37: Fix KeyError and support arrays of primitives in schemas.Accessing schema["items"]["$ref"] assumes both keys exist and misses arrays of primitives (e.g., items.type == "string"). This can raise KeyError and generates wrong types.
- if not self.content[ret_type].get("schema", None): - return "NO_SCHEMA" - - if self.content[ret_type]["schema"].get("type", None) == "array": - is_array = True - ref = self.content[ret_type]["schema"]["items"]["$ref"] - elif self.content[ret_type]["schema"].get("$ref", None): - ref = self.content[ret_type]["schema"]["$ref"] - elif self.content[ret_type]["schema"].get("type", None): - ref = self.content[ret_type]["schema"]["type"] + schema = self.content[ret_type].get("schema", {}) + if not schema: + return "NO_SCHEMA" + + if schema.get("type") == "array": + is_array = True + items = schema.get("items") or {} + if isinstance(items, dict) and "$ref" in items: + ref = items["$ref"] + elif isinstance(items, dict) and "type" in items: + ref = items["type"] + else: + ref = None + elif "$ref" in schema: + ref = schema["$ref"] + elif "type" in schema: + ref = schema["type"] else: ref = "NO_REF"
🧹 Nitpick comments (6)
api_lib/from_specs/objects/paths.py (6)
20-26: Broaden content-type detection (support +json variants and plain text fallback).Right now only exact "application/json" and "text/plain" are recognized. Specs often use vendor or RFC flavors like "application/problem+json", "application/ld+json", etc. Prefer any key that endswith "+json" or "/json" before falling back to "text/plain".
- if self.content.get("application/json", None): - ret_type = "application/json" - elif self.content.get("text/plain", None): - ret_type = "text/plain" - else: - return "NO_APPLICATION_JSON" + json_key = next( + (k for k in self.content.keys() if k == "application/json" or k.endswith("+json") or k.endswith("/json")), + None, + ) + if json_key: + ret_type = json_key + elif "text/plain" in self.content: + ret_type = "text/plain" + else: + return "NO_APPLICATION_JSON"
59-63: Make method naming consistent and safer with snakecase; handle missing operationId.Leverage the shared snakecase helper and guard against None.
- return self.operationId.replace("-", "_").replace(" ", "_").lower() + op = self.operationId or "" + return snakecase(op.replace("-", "_").replace(" ", "_"))
70-73: Prefix stripping should respect path boundaries.Using startswith(prefix) can incorrectly strip "/api" from "/apiv2". Ensure the next char is "/" or the whole path matches.
- use_api_prefix = prefix and path.startswith(prefix) - full_path = path.split(prefix, 1)[1] if use_api_prefix else path + use_api_prefix = bool(prefix) and path.startswith(prefix) + if use_api_prefix and (path == prefix or path.startswith(prefix + "/")): + full_path = path[len(prefix):] + else: + use_api_prefix = False + full_path = path
74-81: Simplify placeholder formatting; avoid nested f-string gymnastics and double snakecase.This improves readability and avoids subtle brace issues.
- for sub_path in sub_paths: - if sub_path.startswith("{") and sub_path.endswith("}"): - params.append(snakecase(f"{sub_path[1:-1]}: str")) - formatted_path.append(snakecase(f"{'{'}{sub_path[1:-1]}{'}'}")) - else: - formatted_path.append(sub_path) + for sub_path in sub_paths: + if sub_path.startswith("{") and sub_path.endswith("}"): + name = snakecase(sub_path[1:-1]) + params.append(f"{name}: str") + formatted_path.append(f"{{{name}}}") + else: + formatted_path.append(sub_path)
85-98: Avoid variable shadowing; build try_req args clearly; use spaces instead of tabs in generated code.Shadowing params and using a literal tab harms readability and consistency.
- first_line: str = f"def {self.method_name}({', '.join(params)}) -> Optional[{ret_type}]:" + first_line: str = f"def {self.method_name}({', '.join(params)}) -> Optional[{ret_type}]:" @@ - params: list[str] = [f"Method.{method.upper()}", f'{'f' if len(params) > 1 else ''}"{path}"', ret_type] + req_args: list[str] = [ + f"Method.{method.upper()}", + f'{"f" if len(params) > 1 else ""}"{path}"', + ret_type, + ] @@ - if not use_api_prefix: - params += ["use_api_prefix=False"] - if ret_type == "bool": - params += ["return_state=True"] + if not use_api_prefix: + req_args.append("use_api_prefix=False") + if ret_type == "bool": + req_args.append("return_state=True") @@ - second_line: str = f"return self.try_req({', '.join(params)})" - return [first_line, f"\t{second_line}\n"] + second_line: str = f"return self.try_req({', '.join(req_args)})" + return [first_line, f" {second_line}\n"]
109-117: Generate methods in a stable, conventional order.Iterating vars(self) can reorder across runs/py versions. Emit in a fixed order to reduce diff noise.
- methods = [] - for method in vars(self): - query: Query = getattr(self, method) - if query is not None: - methods.extend(query.to_method_string(path, method, prefix)) - - return methods + method_order = ("get", "post", "put", "patch", "delete") + methods: list[str] = [] + for m in method_order: + query: Optional[Query] = getattr(self, m, None) + if query is not None: + methods.extend(query.to_method_string(path, m, prefix)) + return methods
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.gitignore(1 hunks)api_lib/from_specs/__main__.py(1 hunks)api_lib/from_specs/lib.py(1 hunks)api_lib/from_specs/objects/paths.py(1 hunks)api_lib/from_specs/objects/specs.py(1 hunks)justfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- justfile
- api_lib/from_specs/lib.py
- .gitignore
- api_lib/from_specs/objects/specs.py
- api_lib/from_specs/main.py
🧰 Additional context used
🧬 Code graph analysis (1)
api_lib/from_specs/objects/paths.py (2)
api_lib/from_specs/lib.py (2)
exported_type(24-45)snakecase(5-8)api_lib/from_specs/objects/parser.py (1)
Parser(7-38)
🔇 Additional comments (1)
api_lib/from_specs/objects/paths.py (1)
12-19: Guard against “NO_*” sentinels leaking into type hints.If schema/content is missing or unsupported, the current flow can produce Optional[NO_SCHEMA]/Optional[NO_APPLICATION_JSON], which is an invalid annotation in generated code. The diffs above normalize to a safe "bool" return type in those cases so the generator keeps producing valid Python.
Do you want me to add unit tests to cover:
- array of primitives (items.type = "string") → list[str]
- array of refs (items.$ref) → list[responses.Model]
- content = "application/problem+json" → treated as JSON
- missing schema/content → default to Optional[bool] and sets return_state=True when not using api prefix?
Also applies to: 27-29, 40-48, 64-69
| ref: str = ref.split("/")[-1].split("Response")[0] if ref else "NO_REF" | ||
|
|
||
| if ref not in ["string", "integer", "boolean"]: | ||
| ref = f"responses.{ref}" | ||
|
|
||
| if is_array: | ||
| return f"list[{ref}]" | ||
| else: | ||
| return ref |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize refs safely; avoid over-stripping “Response”; don’t namespace primitives; emit list[str] for arrays of primitives.
- Use endswith("Response") instead of split("Response") to avoid removing mid-string occurrences.
- Do not prefix primitives (string/integer/boolean/number/object) with "responses." or exported_type() will mis-map.
- For arrays of primitives, return list[str|int|bool|float], not list[string|integer|…].
- ref: str = ref.split("/")[-1].split("Response")[0] if ref else "NO_REF"
-
- if ref not in ["string", "integer", "boolean"]:
- ref = f"responses.{ref}"
-
- if is_array:
- return f"list[{ref}]"
- else:
- return ref
+ # Normalize reference name
+ ref: str = ref.split("/")[-1] if isinstance(ref, str) else "NO_REF"
+ # Drop trailing "Response" suffix only
+ if ref.endswith("Response"):
+ ref = ref[: -len("Response")]
+
+ primitives = {"string", "integer", "boolean", "number", "object"}
+ # Namespace only non-primitive models
+ if ref not in primitives and ref != "NO_REF":
+ ref = f"responses.{ref}"
+
+ if is_array:
+ # Ensure primitives are exported to Python-native types
+ if ref in primitives:
+ return f"list[{exported_type(ref)}]"
+ return f"list[{ref}]"
+ return ref📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ref: str = ref.split("/")[-1].split("Response")[0] if ref else "NO_REF" | |
| if ref not in ["string", "integer", "boolean"]: | |
| ref = f"responses.{ref}" | |
| if is_array: | |
| return f"list[{ref}]" | |
| else: | |
| return ref | |
| # Normalize reference name | |
| ref: str = ref.split("/")[-1] if isinstance(ref, str) else "NO_REF" | |
| # Drop trailing "Response" suffix only | |
| if ref.endswith("Response"): | |
| ref = ref[: -len("Response")] | |
| primitives = {"string", "integer", "boolean", "number", "object"} | |
| # Namespace only non-primitive models | |
| if ref not in primitives and ref != "NO_REF": | |
| ref = f"responses.{ref}" | |
| if is_array: | |
| # Ensure primitives are exported to Python-native types | |
| if ref in primitives: | |
| return f"list[{exported_type(ref)}]" | |
| return f"list[{ref}]" | |
| return ref |
🤖 Prompt for AI Agents
In api_lib/from_specs/objects/paths.py around lines 40 to 48, the current logic
over-strips "Response" using split, incorrectly prefixes primitive types with
"responses.", and returns non-Python primitive names for arrays; update it to:
detect and remove the "Response" suffix only when the ref endswith("Response"),
treat primitives (string, integer, boolean, number, object) as raw types and do
not prefix them with "responses.", map OpenAPI primitives to Python types
(string->str, integer->int, boolean->bool, number->float, object->dict), and
when is_array is true emit list[<mapped_python_type>] (e.g., list[str]) so
arrays of primitives produce list[str|int|bool|float|dict] as appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
api_lib/from_specs/objects/infos.py (1)
4-14: Align Infos models with OpenAPI specificationAccording to the OpenAPI specification, the
descriptionfield and nestedcontact/licenseobjects are optional. The current implementation treats them as required fields, which will cause parsing failures when these optional fields are missing from the spec.Apply this diff to align with OpenAPI spec requirements:
+import re + +@ParserObject +class InfosContact(Parser): + name: str | None = None + email: str | None = None + url: str | None = None + +@ParserObject +class InfosLicense(Parser): + name: str | None = None + identifier: str | None = None # OAS 3.1 + url: str | None = None @ParserObject class Infos(Parser): title: str - description: str + description: str | None = None + contact: InfosContact | None = None + license: InfosLicense | None = None version: str @property def class_title(self) -> str: - # convert - to _ and then convert to CamelCase - return "".join(word.capitalize() for word in self.title.replace("-", "_").split("_")) + """Derive a safe CamelCase class name from the spec title.""" + base = re.sub(r"[^0-9A-Za-z]+", "_", self.title) + parts = [p for p in base.strip("_").split("_") if p] + name = "".join(p[:1].upper() + p[1:] for p in parts) + if name and name[0].isdigit(): + name = "_" + name + return nameapi_lib/from_specs/objects/paths.py (3)
30-32: Add error handling for missing "items" key in array schemasWhen the schema type is "array", the code assumes both "items" and "$ref" keys exist, which could raise a KeyError if the schema is malformed.
if self.content[ret_type]["schema"].get("type", None) == "array": is_array = True - ref = self.content[ret_type]["schema"]["items"]["$ref"] + items = self.content[ret_type]["schema"].get("items", {}) + ref = items.get("$ref", "NO_REF")
40-48: Improve reference normalization and type mappingThe current implementation has issues with stripping "Response" suffix and incorrectly prefixes primitive types.
- ref: str = ref.split("/")[-1].split("Response")[0] if ref else "NO_REF" - - if ref not in ["string", "integer", "boolean"]: - ref = f"responses.{ref}" - - if is_array: - return f"list[{ref}]" - else: - return ref + # Normalize reference name + ref: str = ref.split("/")[-1] if isinstance(ref, str) else "NO_REF" + # Drop trailing "Response" suffix only + if ref.endswith("Response"): + ref = ref[:-len("Response")] + + primitives = {"string", "integer", "boolean", "number", "object"} + # Namespace only non-primitive models + if ref not in primitives and ref != "NO_REF": + ref = f"responses.{ref}" + + if is_array: + # Ensure primitives are exported to Python-native types + if ref in primitives: + from ..lib import exported_type + return f"list[{exported_type(ref)}]" + return f"list[{ref}]" + return ref
71-76: Improve response selection and default handlingThe current implementation non-deterministically selects the first 2xx response and could generate invalid
Optional[None]types.- response = next((resp for code, resp in self.responses.items() if str(code).startswith("2")), None) - - ret_type = exported_type(response.schema) if response else "None" + responses = self.responses or {} + # Prefer 200, then 201, then 204, then any other 2xx + preferred = next((responses.get(str(code)) for code in (200, 201, 204) if str(code) in responses), None) + response = preferred or next((resp for code, resp in responses.items() if str(code).startswith("2")), None) + + ret_schema = response.schema if response else "bool" + # Handle invalid schemas + if isinstance(ret_schema, str) and ret_schema.startswith("NO_"): + ret_schema = "bool" + ret_type = exported_type(ret_schema)
🧹 Nitpick comments (2)
api_lib/from_specs/lib.py (2)
5-8: Edge case in snakecase conversionThe current implementation doesn't handle consecutive uppercase letters correctly (e.g., "XMLParser" becomes "x_m_lparser" instead of "xml_parser").
Consider this improved implementation:
def snakecase(text: str) -> str: if text.islower() or not text: return text - return text[0].lower() + "".join("_" + x.lower() if x.isupper() else x for x in text[1:]) + result = text[0].lower() + for i in range(1, len(text)): + if text[i].isupper(): + # Don't add underscore if previous char was uppercase (handling acronyms) + if i > 0 and not text[i-1].isupper(): + result += "_" + result += text[i].lower() + else: + result += text[i] + return result
42-45: Inconsistent handling of null typesThe function checks for "null" in
_typebut doesn't handle the common OpenAPI case of["string", "null"]arrays correctly.Improve null handling for OpenAPI type arrays:
- if _type and "null" in _type: - return f"Optional[{type_mapping.get(_type, 'str')}]" + if _type and "null" in str(_type): + # Handle array types like ["string", "null"] + if isinstance(_type, list): + non_null_types = [t for t in _type if t != "null"] + if non_null_types: + base_type = type_mapping.get(non_null_types[0], non_null_types[0]) + return f"Optional[{base_type}]" + return f"Optional[{type_mapping.get(str(_type).replace('null', '').strip(), 'str')}]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
api_lib/api_lib.py(1 hunks)api_lib/from_specs/__main__.py(1 hunks)api_lib/from_specs/lib.py(1 hunks)api_lib/from_specs/objects/components.py(1 hunks)api_lib/from_specs/objects/infos.py(1 hunks)api_lib/from_specs/objects/paths.py(1 hunks)api_lib/objects/response.py(0 hunks)
💤 Files with no reviewable changes (1)
- api_lib/objects/response.py
🚧 Files skipped from review as they are similar to previous changes (2)
- api_lib/from_specs/main.py
- api_lib/from_specs/objects/components.py
🧰 Additional context used
🧬 Code graph analysis (3)
api_lib/api_lib.py (1)
test/test_api_lib.py (1)
test_timeout_check_success(68-76)
api_lib/from_specs/objects/paths.py (2)
api_lib/from_specs/lib.py (3)
exported_type(24-45)snakecase(5-8)split_line_into_multiple_lines(48-65)api_lib/from_specs/objects/parser.py (1)
Parser(7-38)
api_lib/from_specs/objects/infos.py (1)
api_lib/from_specs/objects/parser.py (1)
Parser(7-38)
🔇 Additional comments (3)
api_lib/api_lib.py (1)
303-303: Ensure reduced sleep duration doesn’t exceed rate limits
- File:
api_lib/api_lib.py, line 303- Tests for retry logic (e.g. in
test/test_api_lib.py) do not reference or assert against the exact sleep duration, so this change won’t break existing tests.Diff for context:
- await asyncio.sleep(0.25) + await asyncio.sleep(0.1)Please verify that invoking the retry loop with a 0.1 s backoff still complies with your external API’s rate-limiting policies and won’t lead to excessive call volume in production.
api_lib/from_specs/lib.py (2)
35-40: Logic error in type mapping lookupThe loop logic is incorrect - it checks if
keyis NOT in_typeand continues, but should check if key IS in_type.Fix the condition:
if _type and str(_type) not in type_mapping: for key in type_mapping.keys(): - if str(key) not in _type: + if str(key) not in str(_type): continue _type = key breakLikely an incorrect or invalid review comment.
56-63: Potential IndexError in line splitting functionThe while loop accesses
words[0]without checking if the list is empty after the inner while loop.Add boundary check:
index = 0 while len(words) > 0: sub_lines.append("") - while (len(sub_lines[index]) + len(words[0]) + 1) <= length: + while len(words) > 0 and (len(sub_lines[index]) + len(words[0]) + 1) <= length: sub_lines[index] += f" {words.pop(0)}" - if len(words) == 0: - break sub_lines[index] = sub_lines[index].strip() index += 1Likely an incorrect or invalid review comment.
| if directory.exists(): | ||
| for item in directory.iterdir(): | ||
| item.unlink() | ||
| directory.rmdir() |
There was a problem hiding this comment.
Potential data loss with recursive directory deletion
The manage_directories function recursively deletes all items in directories. This could lead to accidental data loss if called with incorrect paths.
Add safety checks and use shutil.rmtree for safer directory removal:
+import shutil
+
def manage_directories(*directories: Path):
"""Create directories if they do not exist, or clear them if they do."""
for directory in directories:
+ # Safety check - prevent accidental deletion of system directories
+ if str(directory.absolute()).startswith(("/", "C:\\", "~")):
+ raise ValueError(f"Refusing to manage system directory: {directory}")
+
if directory.exists():
- for item in directory.iterdir():
- item.unlink()
- directory.rmdir()
+ shutil.rmtree(directory)
directory.mkdir(parents=True, exist_ok=True)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api_lib/from_specs/lib.py around lines 14 to 17, the current logic iterates
and unlinks items then removes the directory, which risks accidental data loss
and fails on nested directories; replace this with a safer removal: validate the
target path first (ensure it is not root, home, or outside an allowed base
directory and is indeed a directory), add an explicit whitelist or base-path
check, and then call shutil.rmtree(directory) to remove recursively; also
consider adding an optional confirmation flag or dry-run parameter to guard
against unintended deletions and raise a clear exception on validation failure.
|
|
||
| if not use_api_prefix: | ||
| params += ["use_api_prefix=False"] | ||
| if ret_type == "bool": | ||
| params += ["return_state=True"] | ||
|
|
There was a problem hiding this comment.
Logic error in conditional parameter addition
The return_state=True parameter is only added when use_api_prefix is False, but it should be based solely on whether ret_type == "bool".
Fix the conditional logic:
if not use_api_prefix:
params += ["use_api_prefix=False"]
- if ret_type == "bool":
- params += ["return_state=True"]
+
+ if ret_type == "bool":
+ params += ["return_state=True"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not use_api_prefix: | |
| params += ["use_api_prefix=False"] | |
| if ret_type == "bool": | |
| params += ["return_state=True"] | |
| if not use_api_prefix: | |
| params += ["use_api_prefix=False"] | |
| if ret_type == "bool": | |
| params += ["return_state=True"] |
🤖 Prompt for AI Agents
In api_lib/from_specs/objects/paths.py around lines 102 to 107, the code only
appends "return_state=True" when use_api_prefix is False because the ret_type ==
"bool" check is nested inside that block; change the logic so
"return_state=True" is appended whenever ret_type == "bool" regardless of
use_api_prefix (i.e., make the ret_type == "bool" check a separate, independent
if that appends to params).
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
Tests