Skip to content
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

refactor(parser validation): Refactor BD part 1 #6265

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

unitrium
Copy link
Contributor

@unitrium unitrium commented Dec 20, 2023

Description

I'm starting a refactor of this parser. To avoid a big messy PR I'm splitting it into smaller PRs.
This PR just renames the parser and create a baseline test which would make sure that subsequent PRs don't break anything.

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier --write . and poetry run format to format my changes.

@github-actions github-actions bot added parser python Pull requests that update Python code tests labels Dec 20, 2023
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Refactoring of the parser for the Power Grid Company of Bangladesh (ERP_PGCB)
  • 📝 PR summary: This PR is the first part of a refactor of the parser for the Power Grid Company of Bangladesh (ERP_PGCB). The parser has been renamed and a baseline test has been created to ensure that subsequent PRs do not break anything. The parser fetches production, consumption, and exchange data.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR involves a significant amount of new code, including a new parser and tests. The code is well-structured and follows good practices, but it still requires a careful review due to its complexity and the importance of the data it handles.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the code is clean and follows good practices. The use of helper functions to handle specific tasks (like parsing table entries and verifying table headers) improves readability and maintainability. The use of a baseline test to ensure the functionality of the parser is a good practice. However, it would be beneficial to add more detailed comments explaining the purpose of certain code blocks or functions, especially for those that handle complex tasks.

  • 🤖 Code feedback:
    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider handling exceptions more gracefully. For instance, in the `table_entry_to_float` function, if the entry cannot be converted to a float, a ParserException is raised. It might be better to log the error and continue with the next entries, or to return a default value, to avoid stopping the entire parsing process because of one problematic entry. [important]
    relevant line'+ raise ParserException('

    relevant fileparsers/ERP_PGCB.py
    suggestion      In the `fetch_production`, `fetch_consumption`, and `fetch_exchange` functions, consider handling the case where `row_data` is empty more gracefully. Currently, a ParserException is raised if no valid data is found for the requested day. It might be better to return an empty list or None, and log a warning, to avoid raising an exception that could potentially stop the entire process. [medium]
    relevant line'+ raise ParserException('

    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider adding more detailed logging throughout the code. While there are some logs for warning cases, adding info-level logs could help with debugging and understanding the flow of the program. For instance, logging the URL being queried, the number of rows parsed, or the number of entries processed could be useful. [medium]
    relevant line'+ logger.warn('

    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider using constants for repeated strings, such as the source URL 'erp.pgcb.gov.bd'. This can help avoid potential errors due to typos and makes it easier to update the value if needed. [medium]
    relevant line'+ "source": "erp.pgcb.gov.bd",'

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@github-actions github-actions bot added zone config Pull request or issue for zone configurations exchange config Pull request or issue for exchange configurations labels Dec 20, 2023
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll mark the flagged code as used in tests later today 🙂

@unitrium
Copy link
Contributor Author

I have dismissed the warnings but I actually agree that I could sanitize the html input a bit, people will execute this test on their computers.

@unitrium unitrium merged commit 2a35fa4 into master Dec 20, 2023
19 checks passed
@unitrium unitrium deleted the robin/refactor-BD-part1 branch December 20, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exchange config Pull request or issue for exchange configurations parser python Pull requests that update Python code tests zone config Pull request or issue for zone configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants