chore: sync template from jebel-quant/rhiza@main#269
Conversation
WalkthroughThis PR restructures the project's build and release automation by introducing new shell scripts for versioning (bump, release), template synchronization, and configuration management; updating GitHub Actions workflows to use dynamic Python version detection and refactoring uv setup; expanding test coverage; and updating configuration files to reflect repository renaming. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (34)
tests/test_jquantstats/conftest.py (1)
170-170: Consider extracting the error message.The inline error message could be extracted to improve maintainability, though this is a minor style preference.
Apply this diff if you prefer:
- raise FileNotFoundError("README.md not found in any parent directory") + msg = "README.md not found in any parent directory" + raise FileNotFoundError(msg)Based on static analysis hints (Ruff TRY003).
tests/test_jquantstats/test_build_data.py (1)
70-89: Consider extracting the error message.The inline error message could be extracted for consistency with best practices, though this is a minor style preference.
Apply this diff if you prefer:
if b is not None: assert b.columns == ["SPY -- Benchmark"] else: - raise AssertionError("No benchmark data available") + msg = "No benchmark data available" + raise AssertionError(msg)Based on static analysis hints (Ruff TRY003).
tests/test_jquantstats/test_build_data_pandas.py (3)
153-180: Consider extracting the error message.The inline error message could be extracted for consistency with best practices, though this is a minor style preference.
Apply this diff if you prefer:
assert b.shape[0] == result.returns.shape[0] else: - raise AssertionError("No benchmark data available") + msg = "No benchmark data available" + raise AssertionError(msg)Based on static analysis hints (Ruff TRY003).
183-210: Consider extracting the error message.The inline error message could be extracted for consistency with best practices, though this is a minor style preference.
Apply this diff if you prefer:
assert b.shape[0] == result.returns.shape[0] else: - raise AssertionError("No benchmark data available") + msg = "No benchmark data available" + raise AssertionError(msg)Based on static analysis hints (Ruff TRY003).
275-312: Consider removing unused parameter.The
rf_series_pdparameter is declared but not used in the test (line 289 usesrf=0.0instead). If the parameter isn't needed, consider removing it from the function signature for clarity.Apply this diff if the parameter is truly unnecessary:
-def test_build_data_with_pd_all_inputs(returns_series_pd, benchmark_series_pd, rf_series_pd): +def test_build_data_with_pd_all_inputs(returns_series_pd, benchmark_series_pd):Based on static analysis hints (Ruff ARG001).
tests/test_jquantstats/test_data.py (3)
162-170: Use raw strings for regex patterns.The
matchparameter accepts a regex pattern. The.character is a metacharacter that matches any character. Use raw strings (r"...") to make the intent clear and avoid accidental escaping issues.- with pytest.raises(ValueError, match="Index must contain at least two timestamps."): + with pytest.raises(ValueError, match=r"Index must contain at least two timestamps\."): build_data(returns=single_returns, date_col="Date") # Test case 2: Unsorted index unsorted_dates = [date(2023, 1, 15), date(2023, 1, 1), date(2023, 1, 30)] unsorted_returns = pl.DataFrame({"Date": unsorted_dates, "returns": [0.01, 0.02, 0.03]}) - with pytest.raises(ValueError, match="Index must be monotonically increasing."): + with pytest.raises(ValueError, match=r"Index must be monotonically increasing\."): build_data(returns=unsorted_returns)
177-191: Use raw strings for regex patterns in remainingpytest.raisescalls.Same issue as above — use raw strings for regex patterns to be explicit.
- with pytest.raises(ValueError, match="Returns and index must have the same number of rows."): + with pytest.raises(ValueError, match=r"Returns and index must have the same number of rows\."): from jquantstats._data import Data Data(returns=returns, index=index) # Test case 4: Benchmark and index with different row counts dates = [date(2023, 1, 1), date(2023, 1, 15), date(2023, 1, 30)] returns = pl.DataFrame({"returns": [0.01, 0.02, 0.03]}) benchmark = pl.DataFrame({"benchmark": [0.01, 0.02]}) index = pl.DataFrame({"Date": dates}) - with pytest.raises(ValueError, match="Benchmark and index must have the same number of rows."): + with pytest.raises(ValueError, match=r"Benchmark and index must have the same number of rows\."): from jquantstats._data import Data Data(returns=returns, benchmark=benchmark, index=index)
301-313: Duplicate test across files.
test_copy_no_benchmarkis also defined intest_edge_cases.py(lines 24-49). Consider consolidating these tests into a single location to avoid maintenance overhead and potential inconsistencies.tests/test_jquantstats/test_edge_cases.py (1)
86-87: Use raw string for regex pattern.The
matchparameter accepts a regex. Use a raw string to be explicit about the pattern.- with pytest.raises(ValueError, match="No overlapping dates between returns and benchmark."): + with pytest.raises(ValueError, match=r"No overlapping dates between returns and benchmark\."): build_data(returns=returns, benchmark=benchmark)tests/test_jquantstats/test_stats.py (1)
117-119: Debug print statements in tests.Consider removing
print()statements from tests. If output is needed for debugging, use pytest's-sflag with proper logging orpytest.fail()with a message for diagnostic failures.Also applies to: 128-128, 394-394, 423-423
.github/scripts/release.sh (3)
136-163: Branch divergence logic is sound but could be slightly clearer.The remote status checking (lines 136–163) properly handles three cases: local behind, local ahead, and diverged branches. The logic is correct, but the indentation inconsistency on line 150 (mixed spacing) and line 151 (extra indentation) makes it slightly harder to read.
Additionally, consider extracting the branch status check into a helper function to reduce cognitive complexity in the main
do_release()function.LOCAL=$(git rev-parse @) REMOTE=$(git rev-parse "$UPSTREAM") BASE=$(git merge-base @ "$UPSTREAM") if [ "$LOCAL" != "$REMOTE" ]; then if [ "$LOCAL" = "$BASE" ]; then - printf "%b[ERROR] Your branch is behind '%s'. Please pull changes.%b\n" "$RED" "$UPSTREAM" "$RESET" - exit 1 + printf "%b[ERROR] Your branch is behind '%s'. Please pull changes.%b\n" "$RED" "$UPSTREAM" "$RESET" + exit 1 elif [ "$REMOTE" = "$BASE" ]; then - printf "%b[WARN] Your branch is ahead of '%s'.%b\n" "$YELLOW" "$UPSTREAM" "$RESET" - printf "Unpushed commits:\n" - git log --oneline --graph --decorate "$UPSTREAM..HEAD" - prompt_continue "Push changes to remote before releasing?" - git push origin "$CURRENT_BRANCH" + printf "%b[WARN] Your branch is ahead of '%s'.%b\n" "$YELLOW" "$UPSTREAM" "$RESET" + printf "Unpushed commits:\n" + git log --oneline --graph --decorate "$UPSTREAM..HEAD" + prompt_continue "Push changes to remote before releasing?" + git push origin "$CURRENT_BRANCH" else - printf "%b[ERROR] Your branch has diverged from '%s'. Please reconcile.%b\n" "$RED" "$UPSTREAM" "$RESET" - exit 1 + printf "%b[ERROR] Your branch has diverged from '%s'. Please reconcile.%b\n" "$RED" "$UPSTREAM" "$RESET" + exit 1 fi fi
207-207: Minor: Use more descriptive prompt message and verify sed regex for repo URL.Line 207 calls
prompt_continue ""with an empty message, resulting in a less-than-clear prompt. Provide a meaningful message like"Push tag to remote and trigger release workflow?".Additionally, line 211's sed regex to extract the repo URL may fail on edge cases:
- SSH URLs:
git@github.com:org/repo.git→ extracts correctly asorg/repo- HTTPS URLs:
https://github.com/org/repo.git→ extracts correctly asorg/repo- But may fail if the URL format is unusual or doesn't follow the standard pattern
Consider more robust handling or explicit error handling.
- prompt_continue "" + prompt_continue "Push tag to remote and trigger release workflow?"For robust URL extraction, consider:
REPO_URL=$(git config --get remote.origin.url | sed -E 's|.*github\.com[:/]([^/]+)/(.+?)(\.git)?$|\1/\2|')Also applies to: 211-211
166-177: Condition logic on line 180 is backwards; should check if tag was NOT skipped.Line 169 sets
SKIP_TAG_CREATE="true"if the tag already exists locally. However, line 180 checksif [ -z "$SKIP_TAG_CREATE" ], which evaluates to false when the variable is set to"true". This works correctly, but the logic is clearer when checking the opposite condition.Consider this small clarity improvement:
- if [ -z "$SKIP_TAG_CREATE" ]; then + if [ -n "$SKIP_TAG_CREATE" ] && [ "$SKIP_TAG_CREATE" = "true" ]; then + printf "%b[INFO] Skipping tag creation (tag already exists locally).%b\n" "$BLUE" "$RESET" + elseHowever, the current logic is functionally correct and works as intended.
.github/workflows/scripts/version_max.py (1)
14-15: Consider documenting the CANDIDATES maintenance requirement.The
CANDIDATESlist is hardcoded and will need manual updates when new Python versions are released (e.g., 3.15). Consider adding a comment noting this maintenance requirement.Apply this diff:
-CANDIDATES = ["3.11", "3.12", "3.13", "3.14"] # extend as needed +CANDIDATES = ["3.11", "3.12", "3.13", "3.14"] # NOTE: Update when new Python versions are released.github/scripts/bump.sh (1)
39-67: Consider extracting duplicate prompt functions to a shared utility.The
prompt_continueandprompt_yes_nofunctions are duplicated in.github/scripts/release.sh(lines 62-90). Consider extracting these to a shared utility script to reduce duplication.You could create
.github/scripts/lib/prompts.sh:#!/bin/sh # Shared prompt utilities prompt_continue() { # ... implementation } prompt_yes_no() { # ... implementation }Then source it in both scripts:
. "$(dirname "$0")/lib/prompts.sh".github/workflows/devcontainer.yml (5)
63-70: Quote variables in shell expressions.Variables should be quoted to prevent globbing and word splitting issues, especially when values might contain spaces or special characters.
Apply this diff:
run: | - REGISTRY="${{ vars.DEVCONTAINER_REGISTRY }}" + REGISTRY="${{ vars.DEVCONTAINER_REGISTRY }}" if [ -z "$REGISTRY" ]; then REGISTRY="ghcr.io" fi - echo "registry=$REGISTRY" >> $GITHUB_OUTPUT + echo "registry=$REGISTRY" >> "$GITHUB_OUTPUT"
80-88: Quote file paths and output redirections.Apply this diff:
if [ ! -f ".devcontainer/devcontainer.json" ]; then - echo "exists=false" >> $GITHUB_OUTPUT + echo "exists=false" >> "$GITHUB_OUTPUT" echo "::warning::No .devcontainer/devcontainer.json found, skipping build" else - echo "exists=true" >> $GITHUB_OUTPUT + echo "exists=true" >> "$GITHUB_OUTPUT" fi
90-92: Quote command substitution and output redirection.Apply this diff:
- run: echo "owner_lc=$(echo '${{ github.repository_owner }}' | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT + run: echo "owner_lc=$(echo '${{ github.repository_owner }}' | tr '[:upper:]' '[:lower:]')" >> "$GITHUB_OUTPUT"
94-121: Quote output redirections and variable references.Apply this diff to line 121:
- echo "image_name=$IMAGE_NAME" >> $GITHUB_OUTPUT + echo "image_name=$IMAGE_NAME" >> "$GITHUB_OUTPUT"
123-128: Quote output redirection.Apply this diff:
run: | SANITIZED_TAG=$(echo "${{ github.ref_name }}-${{ github.sha }}" | tr '/' '-') - echo "sanitized_tag=$SANITIZED_TAG" >> $GITHUB_OUTPUT + echo "sanitized_tag=$SANITIZED_TAG" >> "$GITHUB_OUTPUT"tests/test_rhiza/test_readme.py (2)
19-45: Consider validating matching code/result block counts before execution.The test merges all code blocks and all result blocks independently. If the README has mismatched counts (e.g., 3 code blocks but 2 result blocks), the test may pass or fail silently without clear diagnostics.
code_blocks = CODE_BLOCK.findall(readme_text) result_blocks = RESULT.findall(readme_text) logger.info("Found %d code block(s) and %d result block(s) in README", len(code_blocks), len(result_blocks)) + + if len(code_blocks) != len(result_blocks): + logger.warning( + "Mismatch: %d code blocks vs %d result blocks - verify README structure", + len(code_blocks), + len(result_blocks), + ) code = "".join(code_blocks) # merged code
64-74: Use the module-levelCODE_BLOCKconstant instead of duplicating the regex.Line 68 defines an inline regex pattern that differs slightly from the
CODE_BLOCKconstant (escaped backticks vs raw string). Using the constant improves consistency and maintainability.def test_readme_code_is_syntactically_valid(self, root): """Python code blocks in README should be syntactically valid.""" readme = root / "README.md" content = readme.read_text(encoding="utf-8") - code_blocks = re.findall(r"\`\`\`python\n(.*?)\`\`\`", content, re.DOTALL) + code_blocks = CODE_BLOCK.findall(content) for i, code in enumerate(code_blocks):tests/test_rhiza/test_sync_script.py (3)
106-119: String replacement for modifying sync script is fragile.The exact multi-line string match is brittle - any whitespace or formatting change in sync.sh will break this test. Consider using a more robust approach like sed or regex replacement.
If the sync.sh content changes (even whitespace), this test will silently fail to make the replacement and may produce confusing failures. Consider adding a check:
test_sync_content = sync_content.replace( "# Clone the template repository\n" ... ) + + # Verify the replacement was made + assert "Using local template directory" in test_sync_content, ( + "Failed to patch sync script - the clone block may have changed" + ) test_script.write_text(test_sync_content)
157-158: Replace print statements with logger calls for consistency.The rest of the test uses
logger.debug()for output, but these lines useprint(). Using logger maintains consistency and respects log level settings.- print("STDOUT:", result.stdout) - print("STDERR:", result.stderr) + logger.info("STDOUT: %s", result.stdout) + logger.info("STDERR: %s", result.stderr)
191-196: Moveosimport to module level.The
osmodule is imported inside the test method. For consistency and to follow Python conventions, imports should be at the top of the file.Add at the top of the file with other imports:
from __future__ import annotations +import os import subprocess from pathlib import PathThen remove the inline import:
def test_sync_script_is_executable(self, root): """Sync script should be executable.""" - import os - sync_script = root / ".github" / "scripts" / "sync.sh" assert os.access(sync_script, os.X_OK)Makefile (3)
54-63: Duplicated message for executable vs non-executable script.Lines 56-57 and 59-60 print the same info message regardless of whether the script is executable or requires
/bin/sh. Consider differentiating the messages for clarity.@if [ -x "${CUSTOM_SCRIPTS_FOLDER}/build-extras.sh" ]; then \ - printf "${BLUE}[INFO] Running custom build script from customisations folder...${RESET}\n"; \ + printf "${BLUE}[INFO] Running executable build-extras.sh from customisations folder...${RESET}\n"; \ "${CUSTOM_SCRIPTS_FOLDER}"/build-extras.sh; \ elif [ -f "${CUSTOM_SCRIPTS_FOLDER}/build-extras.sh" ]; then \ - printf "${BLUE}[INFO] Running custom build script from customisations folder...${RESET}\n"; \ + printf "${BLUE}[INFO] Running build-extras.sh via /bin/sh from customisations folder...${RESET}\n"; \ /bin/sh "${CUSTOM_SCRIPTS_FOLDER}/build-extras.sh"; \
134-162: Consider extracting docs logic into a separate script.The
docstarget has grown complex with package auto-detection, template detection, and docformat inference. This aligns with checkmake's warning about body length. Extracting to a script (e.g.,${SCRIPTS_FOLDER}/docs.sh) would improve maintainability and testability.
204-210:lsoutput may include directory prefix.When using
ls -1 "${CUSTOM_SCRIPTS_FOLDER}"/*.sh, the output includes the full path. If you want just filenames for cleaner output, consider usingbasenameorfind.@if [ -d "${CUSTOM_SCRIPTS_FOLDER}" ]; then \ - ls -1 "${CUSTOM_SCRIPTS_FOLDER}"/*.sh 2>/dev/null || printf " (none)\n"; \ + find "${CUSTOM_SCRIPTS_FOLDER}" -maxdepth 1 -name "*.sh" -exec basename {} \; 2>/dev/null | sed 's/^/ /' || printf " (none)\n"; \ else \tests/test_rhiza/conftest.py (2)
28-36:bump_versionsilently returns current version for invalid bump types.If an invalid bump type is passed (not "major", "minor", or "patch"), the function returns the unchanged version without any warning. This might mask bugs in tests.
def bump_version(current, bump_type): major, minor, patch = map(int, current.split('.')) if bump_type == "major": return f"{major + 1}.0.0" elif bump_type == "minor": return f"{major}.{minor + 1}.0" elif bump_type == "patch": return f"{major}.{minor}.{patch + 1}" - return current + raise ValueError(f"Invalid bump type: {bump_type}")
119-119: Potential warning fromgit checkout -b masterif branch exists.After cloning, if the remote's default branch is already
master, runninggit checkout -b mastermay produce a warning or fail. Consider usinggit checkout -B masterto force-create or reset the branch.- subprocess.run(["git", "checkout", "-b", "master"], check=True) + subprocess.run(["git", "checkout", "-B", "master"], check=True).github/workflows/release.yml (3)
83-85: Quote the variable expansion for robustness.Per shellcheck SC2086, double-quote
$TAGto prevent word splitting.- TAG="${GITHUB_REF#refs/tags/}" - echo "tag=$TAG" >> $GITHUB_OUTPUT + TAG="${GITHUB_REF#refs/tags/}" + echo "tag=${TAG}" >> "$GITHUB_OUTPUT"
232-239: Quote variables in shell conditionals.Per shellcheck SC2086, double-quote
$PUBLISH_DEVCONTAINERto prevent word splitting issues.- if [[ "$PUBLISH_DEVCONTAINER" == "true" ]] && [[ -d ".devcontainer" ]]; then + if [[ "${PUBLISH_DEVCONTAINER}" == "true" ]] && [[ -d ".devcontainer" ]]; then
326-336: Unused variableOWNER_LC.Per shellcheck SC2034,
OWNER_LCis defined but not used in this step. It appears the image name already includes the owner. Consider removing the unused variable.run: | - OWNER_LC=$(echo "${{ github.repository_owner }}" | tr '[:upper:]' '[:lower:]') FULL_IMAGE="${{ needs.devcontainer.outputs.image_name }}:${{ needs.tag.outputs.tag }}"tests/test_rhiza/test_git_repo_fixture.py (1)
39-56: Consider parameterizing tool/script executability tests.The two tests (
test_git_repo_mock_tools_are_executableandtest_git_repo_scripts_are_executable) iterate over lists. Using@pytest.mark.parametrizewould provide better test isolation and clearer failure messages.- def test_git_repo_mock_tools_are_executable(self, git_repo): - """Mock tools should be executable.""" - for tool in ["uv"]: - tool_path = git_repo / "bin" / tool - assert os.access(tool_path, os.X_OK), f"{tool} is not executable" + @pytest.mark.parametrize("tool", ["uv"]) + def test_git_repo_mock_tools_are_executable(self, git_repo, tool): + """Mock tools should be executable.""" + tool_path = git_repo / "bin" / tool + assert os.access(tool_path, os.X_OK), f"{tool} is not executable"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/test_jquantstats/resources/benchmark.csvis excluded by!**/*.csvtests/test_jquantstats/resources/meta.csvis excluded by!**/*.csvtests/test_jquantstats/resources/metrics.csvis excluded by!**/*.csvtests/test_jquantstats/resources/portfolio.csvis excluded by!**/*.csv
📒 Files selected for processing (45)
.editorconfig(1 hunks).github/actions/setup-project/action.yml(5 hunks).github/scripts/bump.sh(1 hunks).github/scripts/customisations/build-extras.sh(1 hunks).github/scripts/customisations/post-release.sh(1 hunks).github/scripts/marimushka.sh(2 hunks).github/scripts/release.sh(1 hunks).github/scripts/sync.sh(1 hunks).github/scripts/update-readme-help.sh(1 hunks).github/workflows/book.yml(3 hunks).github/workflows/ci.yml(1 hunks).github/workflows/deptry.yml(1 hunks).github/workflows/devcontainer.yml(1 hunks).github/workflows/marimo.yml(2 hunks).github/workflows/pre-commit.yml(2 hunks).github/workflows/release.yml(7 hunks).github/workflows/scripts/version_matrix.py(1 hunks).github/workflows/scripts/version_max.py(1 hunks).github/workflows/sync.yml(2 hunks).pre-commit-config.yaml(2 hunks)CONTRIBUTING.md(2 hunks)Makefile(7 hunks)pytest.ini(1 hunks)ruff.toml(1 hunks)src/jquantstats/_plots.py(1 hunks)tests/test_jquantstats/conftest.py(1 hunks)tests/test_jquantstats/resources/download.py(1 hunks)tests/test_jquantstats/test_build_data.py(1 hunks)tests/test_jquantstats/test_build_data_pandas.py(1 hunks)tests/test_jquantstats/test_data.py(1 hunks)tests/test_jquantstats/test_edge_cases.py(1 hunks)tests/test_jquantstats/test_plots.py(1 hunks)tests/test_jquantstats/test_quantstats.py(1 hunks)tests/test_jquantstats/test_reports.py(1 hunks)tests/test_jquantstats/test_stats.py(1 hunks)tests/test_jquantstats/test_version.py(1 hunks)tests/test_rhiza/conftest.py(1 hunks)tests/test_rhiza/test_bump_script.py(1 hunks)tests/test_rhiza/test_docstrings.py(1 hunks)tests/test_rhiza/test_git_repo_fixture.py(1 hunks)tests/test_rhiza/test_makefile.py(1 hunks)tests/test_rhiza/test_readme.py(1 hunks)tests/test_rhiza/test_release_script.py(1 hunks)tests/test_rhiza/test_structure.py(1 hunks)tests/test_rhiza/test_sync_script.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
.github/scripts/release.sh (2)
.github/scripts/sync.sh (1)
show_usage(21-40).github/scripts/bump.sh (1)
prompt_continue(39-52)
tests/test_jquantstats/test_quantstats.py (2)
tests/test_jquantstats/conftest.py (2)
data(97-108)benchmark(61-72)src/jquantstats/_stats.py (22)
sharpe(414-431)skew(88-98)kurtosis(101-113)avg_return(116-126)avg_win(129-139)avg_loss(142-152)volatility(155-181)payoff_ratio(184-199)win_loss_ratio(201-208)profit_ratio(211-233)profit_factor(236-251)value_at_risk(254-271)conditional_value_at_risk(274-300)win_rate(303-315)kelly_criterion(352-372)gain_to_pain_ratio(318-335)risk_return_ratio(338-350)best(375-385)worst(388-398)exposure(401-411)sortino(579-596)information_ratio(772-800)
.github/scripts/sync.sh (1)
.github/scripts/release.sh (1)
show_usage(21-29)
tests/test_jquantstats/test_edge_cases.py (5)
src/jquantstats/api.py (1)
build_data(74-265)tests/test_jquantstats/conftest.py (3)
data_no_benchmark(112-122)returns(45-57)benchmark(61-72)tests/test_jquantstats/test_data.py (1)
test_copy_no_benchmark(301-314)src/jquantstats/_data.py (1)
copy(158-169)src/jquantstats/_stats.py (1)
r_squared(725-760)
tests/test_jquantstats/test_reports.py (1)
tests/test_jquantstats/conftest.py (2)
data(97-108)resource_dir(12-14)
.github/scripts/bump.sh (1)
.github/scripts/release.sh (2)
prompt_continue(63-76)prompt_yes_no(79-91)
tests/test_rhiza/test_readme.py (1)
tests/test_rhiza/conftest.py (2)
logger(91-97)root(82-87)
tests/test_rhiza/test_structure.py (1)
tests/test_rhiza/conftest.py (1)
root(82-87)
tests/test_jquantstats/conftest.py (3)
src/jquantstats/api.py (1)
build_data(74-265)tests/test_jquantstats/test_edge_cases.py (1)
data_no_benchmark(11-21)src/jquantstats/_data.py (1)
date_col(82-90)
tests/test_rhiza/test_git_repo_fixture.py (1)
tests/test_rhiza/conftest.py (1)
git_repo(101-158)
tests/test_rhiza/test_makefile.py (1)
tests/test_rhiza/conftest.py (2)
logger(91-97)root(82-87)
tests/test_rhiza/test_release_script.py (1)
tests/test_rhiza/conftest.py (1)
git_repo(101-158)
tests/test_jquantstats/resources/download.py (1)
tests/test_jquantstats/conftest.py (1)
portfolio(76-93)
tests/test_jquantstats/test_plots.py (3)
src/jquantstats/api.py (1)
build_data(74-265)tests/test_jquantstats/conftest.py (2)
data(97-108)returns(45-57)src/jquantstats/_plots.py (1)
plot_snapshot(173-207)
tests/test_jquantstats/test_build_data_pandas.py (2)
src/jquantstats/api.py (1)
build_data(74-265)tests/test_jquantstats/conftest.py (2)
returns(45-57)benchmark(61-72)
tests/test_rhiza/test_bump_script.py (1)
tests/test_rhiza/conftest.py (1)
git_repo(101-158)
🪛 actionlint (1.7.9)
.github/workflows/devcontainer.yml
65-65: shellcheck reported issue in this script: SC2086:info:5:30: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:5:25: Double quote to prevent globbing and word splitting
(shellcheck)
92-92: shellcheck reported issue in this script: SC2086:info:1:90: Double quote to prevent globbing and word splitting
(shellcheck)
97-97: shellcheck reported issue in this script: SC2086:info:24:34: Double quote to prevent globbing and word splitting
(shellcheck)
126-126: shellcheck reported issue in this script: SC2086:info:2:40: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release.yml
83-83: shellcheck reported issue in this script: SC2086:info:2:20: Double quote to prevent globbing and word splitting
(shellcheck)
232-232: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
232-232: shellcheck reported issue in this script: SC2086:info:5:34: Double quote to prevent globbing and word splitting
(shellcheck)
244-244: shellcheck reported issue in this script: SC2086:info:5:30: Double quote to prevent globbing and word splitting
(shellcheck)
262-262: shellcheck reported issue in this script: SC2086:info:1:90: Double quote to prevent globbing and word splitting
(shellcheck)
267-267: shellcheck reported issue in this script: SC2086:info:24:34: Double quote to prevent globbing and word splitting
(shellcheck)
327-327: shellcheck reported issue in this script: SC2034:warning:1:1: OWNER_LC appears unused. Verify use (or export if used externally)
(shellcheck)
327-327: shellcheck reported issue in this script: SC2086:info:9:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 checkmake (0.2.2)
Makefile
[warning] 117-117: Target body for "marimushka" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 134-134: Target body for "docs" exceeds allowed length of 5 (28).
(maxbodylength)
[warning] 204-204: Target body for "customisations" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 Ruff (0.14.8)
tests/test_jquantstats/test_edge_cases.py
86-86: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tests/test_rhiza/test_sync_script.py
70-70: subprocess call: check for execution of untrusted input
(S603)
70-70: Starting a process with a partial executable path
(S607)
144-144: subprocess call: check for execution of untrusted input
(S603)
tests/test_rhiza/conftest.py
108-108: subprocess call: check for execution of untrusted input
(S603)
108-108: Starting a process with a partial executable path
(S607)
110-110: Starting a process with a partial executable path
(S607)
113-113: subprocess call: check for execution of untrusted input
(S603)
113-113: Starting a process with a partial executable path
(S607)
119-119: Starting a process with a partial executable path
(S607)
152-152: Starting a process with a partial executable path
(S607)
153-153: Starting a process with a partial executable path
(S607)
154-154: Starting a process with a partial executable path
(S607)
155-155: Starting a process with a partial executable path
(S607)
156-156: Starting a process with a partial executable path
(S607)
tests/test_jquantstats/conftest.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_jquantstats/test_build_data.py
89-89: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_rhiza/test_git_repo_fixture.py
61-61: Starting a process with a partial executable path
(S607)
72-72: Starting a process with a partial executable path
(S607)
83-83: Starting a process with a partial executable path
(S607)
94-94: Starting a process with a partial executable path
(S607)
105-105: Starting a process with a partial executable path
(S607)
110-110: Starting a process with a partial executable path
(S607)
120-120: Starting a process with a partial executable path
(S607)
tests/test_rhiza/test_makefile.py
1-1: Docstring contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF002)
tests/test_rhiza/test_release_script.py
18-18: subprocess call: check for execution of untrusted input
(S603)
24-24: Starting a process with a partial executable path
(S607)
37-37: Starting a process with a partial executable path
(S607)
40-40: subprocess call: check for execution of untrusted input
(S603)
52-52: Starting a process with a partial executable path
(S607)
53-53: Starting a process with a partial executable path
(S607)
55-55: subprocess call: check for execution of untrusted input
(S603)
69-69: subprocess call: check for execution of untrusted input
(S603)
82-82: Starting a process with a partial executable path
(S607)
83-83: Starting a process with a partial executable path
(S607)
89-89: subprocess call: check for execution of untrusted input
(S603)
105-105: subprocess call: check for execution of untrusted input
(S603)
105-105: Starting a process with a partial executable path
(S607)
108-108: Starting a process with a partial executable path
(S607)
109-109: Starting a process with a partial executable path
(S607)
114-114: Starting a process with a partial executable path
(S607)
115-115: Starting a process with a partial executable path
(S607)
116-116: Starting a process with a partial executable path
(S607)
119-119: subprocess call: check for execution of untrusted input
(S603)
tests/test_jquantstats/test_data.py
100-100: Unused function argument: data
(ARG001)
162-162: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
169-169: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
177-177: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
188-188: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tests/test_jquantstats/test_build_data_pandas.py
180-180: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Unused function argument: rf_series_pd
(ARG001)
tests/test_rhiza/test_bump_script.py
23-23: subprocess call: check for execution of untrusted input
(S603)
34-34: Starting a process with a partial executable path
(S607)
45-45: subprocess call: check for execution of untrusted input
(S603)
52-52: Starting a process with a partial executable path
(S607)
63-63: Starting a process with a partial executable path
(S607)
64-64: Starting a process with a partial executable path
(S607)
71-71: subprocess call: check for execution of untrusted input
(S603)
85-85: subprocess call: check for execution of untrusted input
(S603)
99-99: Starting a process with a partial executable path
(S607)
103-103: subprocess call: check for execution of untrusted input
(S603)
114-114: Starting a process with a partial executable path
(S607)
119-119: subprocess call: check for execution of untrusted input
(S603)
133-133: subprocess call: check for execution of untrusted input
(S603)
🪛 Shellcheck (0.11.0)
.github/scripts/release.sh
[warning] 64-64: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 80-80: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 139-139: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 139-139: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
.github/scripts/bump.sh
[warning] 40-40: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
| with: | ||
| version: "0.9.17" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
astral uv latest version December 2025
💡 Result:
The latest Astral "uv" release is v0.9.15 (released Dec 2, 2025). [1][2]
Sources:
[1] astral-sh/uv GitHub releases — v0.9.15 (Dec 2, 2025).
[2] Astral (astral.sh) project site / blog (project updates).
Pin UV version to an available release; 0.9.17 does not exist yet.
The latest astral uv release as of December 2025 is v0.9.15 (Dec 2). The pinned version 0.9.17 has not been released and should be updated to 0.9.15 or another available version from the astral-sh/setup-uv releases.
🤖 Prompt for AI Agents
In .github/actions/setup-project/action.yml around lines 43 to 46, the workflow
pins astral-sh/setup-uv to a non-existent release (0.9.17); update the pinned
version to a published tag such as v0.9.15 (or the latest available release) in
the uses/with block so the action references an existing release; ensure the tag
includes the correct prefix (e.g., v0.9.15) and commit the change.
| #!/bin/sh | ||
| # Version bump script | ||
| # - Bumps version in pyproject.toml using uv | ||
| # - Optionally commits the changes | ||
| # - Optionally pushes the changes | ||
| # | ||
| # This script is POSIX-sh compatible and follows the style of other scripts | ||
| # in this repository. It uses uv to manage version updates. | ||
|
|
||
| set -e |
There was a problem hiding this comment.
POSIX compliance claim conflicts with implementation.
The script claims to be "POSIX-sh compatible" (line 7) but uses the local keyword in functions (lines 40, 56), which is not defined in POSIX sh. Consider either removing the POSIX compatibility claim or rewriting the functions to avoid local.
Based on learnings, if POSIX compliance is required, rewrite functions to avoid local:
prompt_continue() {
- local message="$1"
+ _prompt_msg="$1"
printf "\n%b[PROMPT] %s Continue? [y/N] %b" "$YELLOW" "$message" "$RESET"Alternatively, if bash/dash is acceptable, update the comment:
-# This script is POSIX-sh compatible and follows the style of other scripts
+# This script requires bash or dash and follows the style of other scripts📝 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.
| #!/bin/sh | |
| # Version bump script | |
| # - Bumps version in pyproject.toml using uv | |
| # - Optionally commits the changes | |
| # - Optionally pushes the changes | |
| # | |
| # This script is POSIX-sh compatible and follows the style of other scripts | |
| # in this repository. It uses uv to manage version updates. | |
| set -e | |
| #!/bin/sh | |
| # Version bump script | |
| # - Bumps version in pyproject.toml using uv | |
| # - Optionally commits the changes | |
| # - Optionally pushes the changes | |
| # | |
| # This script requires bash or dash and follows the style of other scripts | |
| # in this repository. It uses uv to manage version updates. | |
| set -e |
🤖 Prompt for AI Agents
.github/scripts/bump.sh around lines 1-10 (and functions using local at ~40 and
~56): the header claims POSIX-sh compatibility but the functions use the
non-POSIX keyword `local`, causing the mismatch; either remove all uses of
`local` and rewrite functions to avoid relying on local scope (e.g., use
uniquely-named temporary variables, pass values via parameters, or run a
subshell to limit scope) or acknowledge Bash/dash usage by changing the shebang
to a Bash-compatible interpreter (e.g., #!/usr/bin/env bash) and update the
header comment to remove the POSIX claim; pick one approach and apply it
consistently across the script (replace or remove `local` usages or switch
shebang and docstring).
| # Helper function to prompt user to continue | ||
| prompt_continue() { | ||
| local message="$1" | ||
| printf "\n%b[PROMPT] %s Continue? [y/N] %b" "$YELLOW" "$message" "$RESET" | ||
| read -r answer | ||
| case "$answer" in | ||
| [Yy]*) | ||
| return 0 | ||
| ;; | ||
| *) | ||
| printf "%b[INFO] Aborted by user%b\n" "$YELLOW" "$RESET" | ||
| exit 0 | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
Resolve POSIX sh compatibility issues with local keyword.
The script declares #!/bin/sh (POSIX-compatible) but uses the local keyword on lines 64 and 80, which is a bash/ksh extension not available in POSIX sh. This will cause failures on systems where /bin/sh is not bash.
Options:
- Change shebang to
#!/bin/bash(simplest if bash is required) - Remove
localdeclarations and make variables global (works in POSIX sh but less clean) - Use POSIX-compatible scoping patterns (more complex)
Given that related scripts (bump.sh, sync.sh) in the same directory also use these patterns, clarify the intended shell requirement.
Recommended fix: Update shebang to #!/bin/bash:
-#!/bin/sh
+#!/bin/bashAlso applies to: 78-91
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 64-64: In POSIX sh, 'local' is undefined.
(SC3043)
🤖 Prompt for AI Agents
.github/scripts/release.sh around lines 62 to 76 (and also lines 78 to 91): the
script is declared as #!/bin/sh but uses the non-POSIX local keyword which
breaks on systems where /bin/sh is not bash; fix by updating the shebang to
#!/bin/bash to require bash for this script (or, if POSIX sh must be supported,
remove the local declarations and refactor to avoid bash-only features), and
ensure the same shebang change or equivalent refactor is applied to related
scripts (bump.sh, sync.sh) for consistency.
| def download_returns(ticker, period="max", proxy=None): | ||
| """Download historical returns for a specified ticker or tickers. | ||
|
|
||
| This function utilizes | ||
| yfinance to fetch adjusted closing prices and calculates daily percentage changes. | ||
|
|
||
| Args: | ||
| ticker: The stock ticker symbol or list of ticker symbols to fetch the data for. | ||
| period: The time period for the data, given either as a string (default is "max") | ||
| or a pandas DatetimeIndex to fetch data for a specific date range. | ||
| proxy: Optional proxy settings to be used for the HTTP requests. | ||
|
|
||
| Returns: | ||
| pandas.Series or pandas.DataFrame: A Series or DataFrame containing the daily percentage | ||
| changes of adjusted closing prices of the specified ticker(s). The structure depends | ||
| on whether a single ticker or multiple tickers are provided. | ||
|
|
||
| """ | ||
| params = {"tickers": ticker, "auto_adjust": True, "multi_level_index": False, "progress": False, "proxy": proxy} | ||
| if isinstance(period, pd.DatetimeIndex): | ||
| params["start"] = period[0] | ||
| else: | ||
| params["period"] = period | ||
| dframe = yf.download(**params)["Close"].pct_change() | ||
| dframe = dframe.tz_localize(None) | ||
| return dframe | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the exact file and check surrounding context
find . -path "*tests/test_jquantstats/resources/download.py" -type fRepository: tschm/jquantstats
Length of output: 107
🏁 Script executed:
# Look for requirements or setup files to identify yfinance version
find . -name "requirements*.txt" -o -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" | head -20Repository: tschm/jquantstats
Length of output: 77
🏁 Script executed:
# Check if there are any tests or usage examples for download_returns
rg "download_returns" --type py -A 5 -B 2Repository: tschm/jquantstats
Length of output: 2461
🏁 Script executed:
# Read pyproject.toml to check yfinance version
cat pyproject.toml | grep -A 50 "dependencies\|yfinance"Repository: tschm/jquantstats
Length of output: 1513
🏁 Script executed:
# Read the actual file to verify content matches snippet
cat -n ./tests/test_jquantstats/resources/download.pyRepository: tschm/jquantstats
Length of output: 2012
🌐 Web query:
yfinance 0.2.66 multi_level_index=False column structure multi-ticker behavior
💡 Result:
Short answer: yfinance 0.2.66 is known to return multi-level (MultiIndex) columns for multi-ticker downloads — and in practice users have reported that single-ticker calls can also come back with a second ticker level even when you expect a flat index or pass multi_level_index=False. The library docs and issue tracker show this mismatch and community workarounds.
What to know
- There is an open/closed discrepancy: the download API has a multi_level_index option but its default/behavior has differed from the docs and has produced MultiIndex columns unexpectedly. [1][2]
- v0.2.66 is the current v0.2.x release series and contains other fixes; the MultiIndex behavior has been discussed in issues. [3]
Practical workarounds
- Force a flat column index:
- Drop the ticker level: df.columns = df.columns.droplevel(1). (works when columns have nlevels>1). [4]
- Or reshape to a (Date, Ticker) index: df = df.stack(level=1).rename_axis(['Date','Ticker']).reset_index(level=1). [4]
- Alternatively download tickers one-by-one and concat with an explicit 'Ticker' column (avoids the library's multi-column grouping). [4]
References
- GitHub issue reporting docs/behavior mismatch for multi_level_index. [1]
- yfinance docs: multi-level column explanation. [2]
- yfinance changelog / release notes for 0.2.66. [3]
- StackOverflow / community examples showing droplevel / stack workarounds. [4]
If you want, tell me whether you’re getting multi-level columns for a single ticker and paste a small reproducible snippet; I’ll give the exact code to normalize the DataFrame.
🌐 Web query:
pandas tz_localize(None) on already timezone-naive index error
💡 Result:
Cause
- tz_localize(None) is intended to remove timezone from a tz-aware DatetimeIndex (or to localize a tz-naive index when tz is not None). Calling tz_localize(None) when the index is already timezone‑naive does nothing useful and can raise because pandas expects you to only "remove" tz from tz‑aware objects — check the pandas behavior in the docs. [1][2]
Quick fixes
- Only call tz_localize(None) when the index is tz‑aware:
idx = idx.tz_localize(None) if idx.tz is not None else idx - Or use a guard function:
def remove_tz(idx):
return idx.tz_localize(None) if getattr(idx, "tz", None) is not None else idx - Alternatively, to force convert to UTC then drop tz from any index that might be tz-aware:
idx = idx.tz_convert("UTC").tz_localize(None) # only valid if idx.tz is not None
Reference / further reading
- pandas time zone docs (tz_localize / tz_convert). [1]
- Example/diagnosis and SO discussion of the "Already tz-aware / cannot convert tz-naive" confusion. [2]
Sources
[1] pandas time series docs: tz_localize / tz_convert.
[2] StackOverflow discussion on tz_localize vs tz_convert and the "already tz-aware / tz-naive" errors.
🏁 Script executed:
# Check if there are any other usages or tests that show expected behavior
rg "yf\.download\|multi_level_index\|tz_localize" --type py -B 2 -A 2Repository: tschm/jquantstats
Length of output: 43
🏁 Script executed:
# Let's verify one more thing: does yfinance return timezone-aware or tz-naive indices
# by checking if there are any related issues or documentation in the repo
find . -name "*.md" -o -name "*.txt" | xargs grep -l "yfinance\|timezone\|tz_" 2>/dev/null | head -5Repository: tschm/jquantstats
Length of output: 43
🏁 Script executed:
# Check if there are any existing test data files that show what the expected output structure is
find tests -name "*.csv" -o -name "*meta*" -o -name "*benchmark*" -o -name "*portfolio*" 2>/dev/null | head -10Repository: tschm/jquantstats
Length of output: 354
🏁 Script executed:
# Examine one of the output CSVs to understand the expected structure
head -5 tests/test_jquantstats/resources/meta.csvRepository: tschm/jquantstats
Length of output: 178
multi_level_index=False + ["Close"] indexing is unsafe; also tz_localize(None) and DatetimeIndex handling lack defensive checks.
With multiple tickers, yfinance 0.2.66 can still return MultiIndex columns despite multi_level_index=False, making line 31's ["Close"] indexing vulnerable to KeyError. Additionally, line 32 calls tz_localize(None) without checking if the index is already timezone-naive, which can raise an error. When period is a DatetimeIndex, only start is set (no end), and accessing period[0] on an empty index will crash.
Suggested fixes:
- Remove
multi_level_index=Falseor add defensive column handling to account for flattened/MultiIndex structures - Check
idx.tz is not Nonebefore callingtz_localize(None) - For
DatetimeIndexinput, set bothstartandend; validate non-empty index before accessingperiod[0]
🤖 Prompt for AI Agents
In tests/test_jquantstats/resources/download.py around lines 8 to 34, the
download_returns implementation is unsafe: keep or remove multi_level_index and
defend against MultiIndex columns instead of blindly indexing ["Close"], check
the provided DatetimeIndex is non-empty and set both start and end when period
is a DatetimeIndex, and only call tz_localize(None) if the index has a timezone
(i.e., idx.tz is not None). Fix by either removing multi_level_index=False or by
detecting and normalizing MultiIndex/flattened column layouts to reliably
extract the Close prices, validate period is non-empty before using period[0]
and set params["start"] and params["end"] from the DatetimeIndex, and guard
tz_localize(None) behind a tz check so it runs only when the index is
timezone-aware.
| stock = download_returns("META") | ||
| stock.to_csv("meta.csv") | ||
|
|
||
| spy = download_returns("SPY") | ||
| spy.to_csv("benchmark.csv") | ||
|
|
||
| portfolio = download_returns(ticker=["AAPL", "META"]) | ||
| portfolio.to_csv("portfolio.csv") |
There was a problem hiding this comment.
Writing CSVs to CWD is foot-gun; write next to the script (test resources).
As written, running from repo root will create/overwrite meta.csv/benchmark.csv/portfolio.csv in the root, not in tests/test_jquantstats/resources/, which is likely where the committed fixtures live.
Suggested tweak:
- stock = download_returns("META")
- stock.to_csv("meta.csv")
+ out_dir = Path(__file__).resolve().parent
+
+ stock = download_returns("META")
+ stock.to_csv(out_dir / "meta.csv", index_label="Date")
@@
- spy = download_returns("SPY")
- spy.to_csv("benchmark.csv")
+ spy = download_returns("SPY")
+ spy.to_csv(out_dir / "benchmark.csv", index_label="Date")
@@
- portfolio = download_returns(ticker=["AAPL", "META"])
- portfolio.to_csv("portfolio.csv")
+ portfolio = download_returns(ticker=["AAPL", "META"])
+ portfolio.to_csv(out_dir / "portfolio.csv", index_label="Date")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_jquantstats/resources/download.py around lines 35 to 42, the
script writes CSVs to the current working directory which can overwrite files in
the repo root; change it to write files next to the script by resolving the
script directory (e.g. output_dir = Path(__file__).resolve().parent), then write
using output_dir / "meta.csv", output_dir / "benchmark.csv", output_dir /
"portfolio.csv" (ensuring the directory exists if needed), so fixtures are
created inside tests/test_jquantstats/resources instead of CWD.
| def test_all(data): | ||
| """Tests that the all property returns a DataFrame with all data. | ||
|
|
||
| Args: | ||
| data (_Data): The data fixture containing a Data object. | ||
|
|
||
| Verifies: | ||
| The all property returns a DataFrame that includes all data. | ||
|
|
||
| """ | ||
| print(data.returns.head(5)) | ||
| x = data.all | ||
| print(x) |
There was a problem hiding this comment.
Test lacks assertions.
test_all only prints data but doesn't assert anything meaningful about the all property. Consider adding an assertion to verify expected behavior.
def test_all(data):
"""Tests that the all property returns a DataFrame with all data.
Args:
data (_Data): The data fixture containing a Data object.
Verifies:
The all property returns a DataFrame that includes all data.
"""
- print(data.returns.head(5))
x = data.all
- print(x)
+ assert x is not None
+ assert isinstance(x, pl.DataFrame)
+ # Verify the all property includes returns columns
+ for col in data.returns.columns:
+ assert col in x.columns🤖 Prompt for AI Agents
In tests/test_jquantstats/test_data.py around lines 44 to 56, the test_all
function only prints the Data object's data and does not assert any behavior;
add meaningful assertions to validate the .all property. Specifically, assert
that data.all is a pandas DataFrame, has the expected columns (e.g., includes
the returns/price columns provided by the fixture), and that its row count
matches the source series (or matches concatenation of the underlying series);
optionally assert equality with an expected DataFrame constructed from the
fixture or that key values (first/last rows or head) match known values from
data. Ensure the assertions use pytest-style asserts and remain deterministic
given the test fixture.
| def test_periods_edge_cases(data): | ||
| """Tests edge cases for the _periods_per_year property. | ||
|
|
||
| Args: | ||
| data (Data): The data fixture containing a Data object. | ||
|
|
||
| Verifies: | ||
| 1. ValueError is raised when index has less than 2 timestamps | ||
| 2. Different frequencies return different period counts | ||
| 3. Unsorted data is handled correctly | ||
|
|
||
| """ | ||
| # Weekly data | ||
| # Create dates with weekly intervals | ||
| weekly_dates = [ | ||
| date(2023, 1, 1), | ||
| date(2023, 1, 8), | ||
| date(2023, 1, 15), | ||
| date(2023, 1, 22), | ||
| date(2023, 1, 29), | ||
| date(2023, 2, 5), | ||
| date(2023, 2, 12), | ||
| date(2023, 2, 19), | ||
| date(2023, 2, 26), | ||
| date(2023, 3, 5), | ||
| ] | ||
| weekly_returns = pl.DataFrame({"Date": weekly_dates, "returns": [0.01] * 10}) | ||
| weekly_data = build_data(returns=weekly_returns) | ||
| print(weekly_data._periods_per_year) | ||
| assert weekly_data._periods_per_year == pytest.approx(52.142857142857146) | ||
| # Monthly data | ||
| # Create dates with monthly intervals | ||
| monthly_dates = [ | ||
| date(2023, 1, 1), | ||
| date(2023, 2, 1), | ||
| date(2023, 3, 1), | ||
| date(2023, 4, 1), | ||
| date(2023, 5, 1), | ||
| date(2023, 6, 1), | ||
| date(2023, 7, 1), | ||
| date(2023, 8, 1), | ||
| date(2023, 9, 1), | ||
| date(2023, 10, 1), | ||
| ] | ||
| monthly_returns = pl.DataFrame({"Date": monthly_dates, "returns": [0.01] * 10}) | ||
| monthly_data = build_data(returns=monthly_returns) | ||
| assert monthly_data._periods_per_year == pytest.approx(12.032967032967033) |
There was a problem hiding this comment.
Unused data parameter.
The data fixture is passed but never used in test_periods_edge_cases. Either remove it from the function signature or use it in the test.
-def test_periods_edge_cases(data):
+def test_periods_edge_cases():📝 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.
| def test_periods_edge_cases(data): | |
| """Tests edge cases for the _periods_per_year property. | |
| Args: | |
| data (Data): The data fixture containing a Data object. | |
| Verifies: | |
| 1. ValueError is raised when index has less than 2 timestamps | |
| 2. Different frequencies return different period counts | |
| 3. Unsorted data is handled correctly | |
| """ | |
| # Weekly data | |
| # Create dates with weekly intervals | |
| weekly_dates = [ | |
| date(2023, 1, 1), | |
| date(2023, 1, 8), | |
| date(2023, 1, 15), | |
| date(2023, 1, 22), | |
| date(2023, 1, 29), | |
| date(2023, 2, 5), | |
| date(2023, 2, 12), | |
| date(2023, 2, 19), | |
| date(2023, 2, 26), | |
| date(2023, 3, 5), | |
| ] | |
| weekly_returns = pl.DataFrame({"Date": weekly_dates, "returns": [0.01] * 10}) | |
| weekly_data = build_data(returns=weekly_returns) | |
| print(weekly_data._periods_per_year) | |
| assert weekly_data._periods_per_year == pytest.approx(52.142857142857146) | |
| # Monthly data | |
| # Create dates with monthly intervals | |
| monthly_dates = [ | |
| date(2023, 1, 1), | |
| date(2023, 2, 1), | |
| date(2023, 3, 1), | |
| date(2023, 4, 1), | |
| date(2023, 5, 1), | |
| date(2023, 6, 1), | |
| date(2023, 7, 1), | |
| date(2023, 8, 1), | |
| date(2023, 9, 1), | |
| date(2023, 10, 1), | |
| ] | |
| monthly_returns = pl.DataFrame({"Date": monthly_dates, "returns": [0.01] * 10}) | |
| monthly_data = build_data(returns=monthly_returns) | |
| assert monthly_data._periods_per_year == pytest.approx(12.032967032967033) | |
| def test_periods_edge_cases(): | |
| """Tests edge cases for the _periods_per_year property. | |
| Args: | |
| data (Data): The data fixture containing a Data object. | |
| Verifies: | |
| 1. ValueError is raised when index has less than 2 timestamps | |
| 2. Different frequencies return different period counts | |
| 3. Unsorted data is handled correctly | |
| """ | |
| # Weekly data | |
| # Create dates with weekly intervals | |
| weekly_dates = [ | |
| date(2023, 1, 1), | |
| date(2023, 1, 8), | |
| date(2023, 1, 15), | |
| date(2023, 1, 22), | |
| date(2023, 1, 29), | |
| date(2023, 2, 5), | |
| date(2023, 2, 12), | |
| date(2023, 2, 19), | |
| date(2023, 2, 26), | |
| date(2023, 3, 5), | |
| ] | |
| weekly_returns = pl.DataFrame({"Date": weekly_dates, "returns": [0.01] * 10}) | |
| weekly_data = build_data(returns=weekly_returns) | |
| print(weekly_data._periods_per_year) | |
| assert weekly_data._periods_per_year == pytest.approx(52.142857142857146) | |
| # Monthly data | |
| # Create dates with monthly intervals | |
| monthly_dates = [ | |
| date(2023, 1, 1), | |
| date(2023, 2, 1), | |
| date(2023, 3, 1), | |
| date(2023, 4, 1), | |
| date(2023, 5, 1), | |
| date(2023, 6, 1), | |
| date(2023, 7, 1), | |
| date(2023, 8, 1), | |
| date(2023, 9, 1), | |
| date(2023, 10, 1), | |
| ] | |
| monthly_returns = pl.DataFrame({"Date": monthly_dates, "returns": [0.01] * 10}) | |
| monthly_data = build_data(returns=monthly_returns) | |
| assert monthly_data._periods_per_year == pytest.approx(12.032967032967033) |
🧰 Tools
🪛 Ruff (0.14.8)
100-100: Unused function argument: data
(ARG001)
🤖 Prompt for AI Agents
In tests/test_jquantstats/test_data.py around lines 100 to 146, the test
function test_periods_edge_cases accepts an unused fixture parameter `data`;
remove the unused parameter from the function signature (change def
test_periods_edge_cases(data): to def test_periods_edge_cases():) or
alternatively use the fixture inside the test if it was intended (e.g., replace
local build_data usage with the provided `data` fixture), then run the test
suite to confirm no parameter mismatch errors remain.
| def test_greeks(stats): | ||
| """Tests that the greeks method calculates alpha and beta correctly. | ||
|
|
||
| Args: | ||
| stats: The stats fixture containing a Stats object. | ||
|
|
||
| Verifies: | ||
| The method executes without errors and returns a result. | ||
|
|
||
| """ | ||
| result = stats.greeks(periods_per_year=252) | ||
| print(result) | ||
|
|
||
| # | ||
| # assert isinstance(result, pd.DataFrame) | ||
| # assert "alpha" in result.index | ||
| # assert "beta" in result.index | ||
| # | ||
| # assert result["Benchmark"]["beta"] == 1.0 | ||
| # assert result["Benchmark"]["alpha"] == 0.0 | ||
| # | ||
| # assert result["AAPL"]["beta"] == pytest.approx(1.1090322781954098) | ||
| # assert result["AAPL"]["alpha"] == pytest.approx(0.1576003006124853) | ||
|
|
There was a problem hiding this comment.
Test lacks assertions — only prints result.
test_greeks executes the method but all assertions are commented out. Either restore the assertions or add new ones to validate the expected behavior.
def test_greeks(stats):
"""Tests that the greeks method calculates alpha and beta correctly.
Args:
stats: The stats fixture containing a Stats object.
Verifies:
The method executes without errors and returns a result.
"""
result = stats.greeks(periods_per_year=252)
- print(result)
-
- #
- # assert isinstance(result, pd.DataFrame)
- # assert "alpha" in result.index
- # assert "beta" in result.index
- #
- # assert result["Benchmark"]["beta"] == 1.0
- # assert result["Benchmark"]["alpha"] == 0.0
- #
- # assert result["AAPL"]["beta"] == pytest.approx(1.1090322781954098)
- # assert result["AAPL"]["alpha"] == pytest.approx(0.1576003006124853)
+ assert result is not None
+ # Add assertions for expected structure and values🤖 Prompt for AI Agents
In tests/test_jquantstats/test_stats.py around lines 469 to 492, the test_greeks
function currently only prints the result and has all assertions commented out;
restore or add concrete assertions to validate output (e.g., check result type,
presence of "alpha" and "beta" rows/indexes, and numeric values for Benchmark
and AAPL using pytest.approx for floating comparisons) so the test actually
verifies correctness and fails on regressions.
| def test_r_squared(stats): | ||
| """Tests that the r_squared method calculates R-squared correctly. | ||
|
|
||
| Args: | ||
| stats: The stats fixture containing a Stats object. | ||
|
|
||
| Verifies: | ||
| The method executes without errors and returns a result. | ||
|
|
||
| """ | ||
| result = stats.r_squared() | ||
| print(result) | ||
| # assert 0 <= result <= 1 # R-squared should be between 0 and 1 |
There was a problem hiding this comment.
Test lacks meaningful assertion.
test_r_squared only prints the result with a commented-out assertion. Add a proper assertion to verify expected behavior.
def test_r_squared(stats):
"""Tests that the r_squared method calculates R-squared correctly.
...
"""
result = stats.r_squared()
- print(result)
- # assert 0 <= result <= 1 # R-squared should be between 0 and 1
+ assert result is not None
+ # R-squared values should be between 0 and 1 for each asset
+ for asset in stats.data.assets:
+ if asset in result:
+ assert 0 <= result[asset] <= 1📝 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.
| def test_r_squared(stats): | |
| """Tests that the r_squared method calculates R-squared correctly. | |
| Args: | |
| stats: The stats fixture containing a Stats object. | |
| Verifies: | |
| The method executes without errors and returns a result. | |
| """ | |
| result = stats.r_squared() | |
| print(result) | |
| # assert 0 <= result <= 1 # R-squared should be between 0 and 1 | |
| def test_r_squared(stats): | |
| """Tests that the r_squared method calculates R-squared correctly. | |
| Args: | |
| stats: The stats fixture containing a Stats object. | |
| Verifies: | |
| The method executes without errors and returns a result. | |
| """ | |
| result = stats.r_squared() | |
| assert result is not None | |
| # R-squared values should be between 0 and 1 for each asset | |
| for asset in stats.data.assets: | |
| if asset in result: | |
| assert 0 <= result[asset] <= 1 |
🤖 Prompt for AI Agents
In tests/test_jquantstats/test_stats.py around lines 494 to 506, the
test_r_squared function only prints the result and has a commented-out
assertion; replace the print with a meaningful assertion: ensure the returned
value is not None (or is a float) and assert the expected numeric properties
(e.g., assert isinstance(result, (int, float)) and assert 0.0 <= result <= 1.0)
so the test verifies R-squared falls within the valid range.
| """Tests for the Makefile targets and help output using safe dry‑runs. | ||
|
|
||
| These tests validate that the Makefile exposes expected targets and emits | ||
| the correct commands without actually executing them, by invoking `make -n` | ||
| (dry-run). We also pass `-s` to reduce noise in CI logs. This approach keeps | ||
| tests fast, portable, and free of side effects like network or environment | ||
| changes. | ||
| """ |
There was a problem hiding this comment.
Fix non-breaking hyphen in docstring.
The docstring contains a non-breaking hyphen (‑) instead of a regular hyphen (-) in "dry‑run" and "dry‑runs". This can cause issues with text processing.
Apply this diff:
-"""Tests for the Makefile targets and help output using safe dry‑runs.
+"""Tests for the Makefile targets and help output using safe dry-runs.
These tests validate that the Makefile exposes expected targets and emits
the correct commands without actually executing them, by invoking `make -n`
-(dry-run). We also pass `-s` to reduce noise in CI logs. This approach keeps
+(dry-run). We also pass `-s` to reduce noise in CI logs. This approach keeps
tests fast, portable, and free of side effects like network or environment
changes.
"""📝 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.
| """Tests for the Makefile targets and help output using safe dry‑runs. | |
| These tests validate that the Makefile exposes expected targets and emits | |
| the correct commands without actually executing them, by invoking `make -n` | |
| (dry-run). We also pass `-s` to reduce noise in CI logs. This approach keeps | |
| tests fast, portable, and free of side effects like network or environment | |
| changes. | |
| """ | |
| """Tests for the Makefile targets and help output using safe dry-runs. | |
| These tests validate that the Makefile exposes expected targets and emits | |
| the correct commands without actually executing them, by invoking `make -n` | |
| (dry-run). We also pass `-s` to reduce noise in CI logs. This approach keeps | |
| tests fast, portable, and free of side effects like network or environment | |
| changes. | |
| """ |
🧰 Tools
🪛 Ruff (0.14.8)
1-1: Docstring contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF002)
🤖 Prompt for AI Agents
In tests/test_rhiza/test_makefile.py around lines 1 to 8, the docstring uses
non‑breaking hyphens (U+2011) in "dry‑run" and "dry‑runs"; replace those with
standard ASCII hyphens (-) so the text reads "dry-run" and "dry-runs" to avoid
text-processing issues.
1 participant
This PR syncs the template from:
jebel-quant/rhiza @ main
Summary by CodeRabbit
New Features
bump,release,post-release,sync,update-readmeBug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.