Skip to content

add solution#83

Open
taniakolesnik wants to merge 11 commits into
mate-academy:mainfrom
taniakolesnik:main
Open

add solution#83
taniakolesnik wants to merge 11 commits into
mate-academy:mainfrom
taniakolesnik:main

Conversation

@taniakolesnik

Copy link
Copy Markdown

No description provided.

@taniakolesnik

Copy link
Copy Markdown
Author

test

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This is a great start, and your workflow is well-structured, meeting most of the requirements. However, changes are needed because a configuration issue will cause the workflow to fail. In your .github/workflows/workflows.yml file, you've set a default working-directory of ./src/ for all run steps. This will cause the 'Install dependencies' step to fail because the pip install -r requirements.txt command will be executed from within the ./src/ directory, where the requirements.txt file doesn't exist. To fix this, please remove the global defaults block and instead add the working-directory: ./src/ property only to the specific steps that require it, such as your testing and linting steps.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread .github/workflows/workflows.yml Outdated
Comment on lines +22 to +24
defaults:
run:
working-directory: ./src/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting a default working-directory for all run steps at the job level will likely cause the 'Install dependencies' step to fail. The pip install -r requirements.txt command will be executed from within the ./src/ directory, but the requirements.txt file is typically in the project root. Consider removing this defaults block and adding the working-directory: ./src/ property to only the steps that need it (e.g., tests, coverage, linting).

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great job on this workflow! I'm happy to approve your submission as it meets all the requirements. You've also done well to address the previous feedback on the working-directory configuration.

As a small suggestion for improvement, you can make the workflow more efficient by removing the Run tests step in .github/workflows/workflows.yml. The next step, Coverage report, already runs the tests with pytest --cov, so the first test run is redundant.

This is just a minor optimization, and overall, this is excellent work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +36 to +38
- name: Run tests
working-directory: ./src/
run: pytest -v

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step is redundant because the next step, Coverage report, also runs the tests with pytest. Running the tests twice is inefficient. You can safely remove this Run tests step and let the coverage step handle both testing and reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants