-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
solution #1718
base: master
Are you sure you want to change the base?
solution #1718
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort so far! 😊 However, there are some significant issues that need to be addressed. You have several instances of repeated code which violates the DRY principle. Also, there are some functions that are not used correctly. Please take a look at the inline comments for more details. Keep up the good work, you're almost there! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/main.py
Outdated
def format_single_linter_file(file_path: str, errors: list) -> dict: | ||
# write your code here | ||
return {"errors": | ||
[{ | ||
"line": error.get("line_number"), | ||
"column": error.get("column_number"), | ||
"message": error.get("text"), | ||
"name": error.get("code"), | ||
"source": "flake8"} for error in errors], | ||
"path": file_path, | ||
"status": "passed" | ||
if errors == [] | ||
else "failed" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you're duplicating the code from format_linter_error
function inside format_single_linter_file
function. Instead, you should use format_linter_error
function to format each error in the list. This would make your code more DRY and easier to maintain.
app/main.py
Outdated
def format_linter_report(linter_report: dict) -> list: | ||
# write your code here | ||
return [ | ||
{"errors": [], "path": list(linter_report)[0], "status": "passed"}, | ||
{ | ||
"errors": [ | ||
{ | ||
"line": linter_report.get("line_number"), | ||
"column": linter_report.get("column_number"), | ||
"message": linter_report.get("text"), | ||
"name": linter_report.get("code"), | ||
"source": "flake8"} | ||
for errors in linter_report], | ||
"path": list(linter_report)[1], | ||
"status": "passed" | ||
if "errors" == [] | ||
else "failed"}, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not using the format_single_linter_file
function in the format_linter_report
function. This leads to code duplication and makes the code harder to maintain. Use the format_single_linter_file
function to format each file in the report.
app/main.py
Outdated
if "errors" == [] | ||
else "failed"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are comparing a string 'errors' to an empty list, which will always result in False
. It seems like you wanted to check if the list of errors is empty. To do that, you should refer to the actual variable that holds the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your work so far! 😊 However, there are a few issues that need to be addressed. You are currently hardcoding file paths and their statuses. Also, you are not using your format_single_linter_file
function correctly. Please make sure to pass the correct arguments to this function. Lastly, you have some unnecessary pass
statements in your code that can be removed. Keep going! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/main.py
Outdated
for errors in linter_report], | ||
"path": list(linter_report)[1], | ||
"status": "passed" | ||
if "errors" == [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are comparing a string 'errors' with an empty list. This condition will never be true. It seems like you wanted to check if there are any errors in your report. You should adjust your condition to correctly check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your Pull Request! 🎉 While there are a few issues to address, such as the incorrect use of 'any' with a non-iterable, a mistake in comprehension syntax, and unnecessary 'pass' statements, these are excellent learning opportunities. Remember to review the comments to improve your code. Addressing these issues in future projects will help you grow as a developer. Keep up the good work and continue learning from your experiences!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
No description provided.