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 2 #6267

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

unitrium
Copy link
Contributor

To be reviewed after the other PR to fix names has been merged.

Description

  • Remove arrow.
  • Fix an issue regarding the date. The source is reporting 24:00 as the first hour of the day instead of 00:00. Used with arrow's get it caused confusion because we mapped it to the day before (reflected in the snapshot changes). Now we correctly map it as the hour of the next day.
  • Convert to using TotalConsumption.

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 labels Dec 20, 2023
Base automatically changed from robin/fix-naming to master December 20, 2023 15:11
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Refactoring the parser for the Power Grid Company of Bangladesh
  • 📝 PR summary: This PR refactors the parser for the Power Grid Company of Bangladesh. It removes the use of the arrow library, fixes an issue with the date parsing, and converts to using TotalConsumption. The PR also includes changes to variable names and the structure of the code for better readability and maintainability.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR includes a significant amount of changes to the parser, including changes to the logic of the code, which requires a careful review to ensure that the functionality of the parser is not affected.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the changes are well-documented in the PR description. The changes improve the readability and maintainability of the code. However, it would be beneficial to add tests to ensure that the changes do not break the functionality of the parser.

  • 🤖 Code feedback:
    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider using constants for string literals that are used multiple times in the code, such as the date and time formats ("%d-%m-%Y", "%H:%M:%S", etc.). This can help avoid potential errors due to typos and make the code easier to maintain. [medium]
    relevant lineparsed_day = datetime.strptime(row_items[0], "%d-%m-%Y").date()

    relevant fileparsers/ERP_PGCB.py
    suggestion      It would be beneficial to handle exceptions where they occur. In the current implementation, if an exception occurs while parsing the time, it is caught and then the time is parsed again in a different format. Instead, consider handling the exception where it occurs, i.e., when parsing the time in the first place. This can make the code cleaner and easier to understand. [medium]
    relevant lineexcept ValueError:

    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider using a more descriptive name for the variable 'row_data'. The current name does not clearly indicate what the variable represents. A more descriptive name can make the code easier to understand. [medium]
    relevant linerow_data = []

    relevant fileparsers/ERP_PGCB.py
    suggestion      Consider adding error handling for the case where 'row_items[1]' is not a string. Currently, an assertion is used to check if 'row_items[1]' is a string, but if it is not, an AssertionError will be raised, which is not very informative. Consider raising a custom exception with a more informative error message. [important]
    relevant lineassert isinstance(row_items[1], str)

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 the tests label Dec 20, 2023
@unitrium
Copy link
Contributor Author

Can be reviewed now, the other part has been merged

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.

Everything works as expected.
There is a small type error though but it should not impact runtime functionality, would be good to double check this in the follow up PR for production parsing though.

@@ -166,7 +164,7 @@ def query(
parser="BD.py",
message=("Could not find table header in returned HTML."),
)
verify_table(table_head)
verify_table_header(table_head)
Copy link
Member

Choose a reason for hiding this comment

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

This does cause some type errors as the table head can be of Tag | NavigatableString | Int but I don't think it will cause any runtime issues as it is right now.

row_data.append(
{
"time": datetime.combine(parsed_day, parsed_time, tzinfo=TIMEZONE),
"total_generation": table_entry_to_float(row_items[2]), # MW
Copy link
Member

Choose a reason for hiding this comment

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

Shall we also add a parser function for this?
I know it's not really used by us right now but would be nice to start collecting these to use in validation rules in the future?

@unitrium unitrium merged commit f39e139 into master Dec 21, 2023
19 checks passed
@unitrium unitrium deleted the robin/refactor-BD-part-2 branch December 21, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser python Pull requests that update Python code tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants