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

chore: improve error message for missing header in input csv files #498

Merged
merged 7 commits into from
May 31, 2024

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented May 28, 2024

ECALC-773

Have you remembered and considered?

  • I have remembered to update documentation
  • I have remembered to update manual changelog (docs/docs/changelog/next.md)
  • I have remembered to update migration guide (docs/docs/migration_guides/)
  • I have committed with BREAKING: in footer or ! in header, if breaking
  • I have added tests (if not, comment why)
  • I have used conventional commits syntax (if you squash, make sure that conventional commit is used)
  • I have included the Jira issue ID somewhere in the commit body (ECALC-XXXX)

Why is this pull request needed?

eCalc correctly points to error in csv file, when missing headers. The name of the file is not given. This can make it difficult /time consuming for the user to debug - if many files.

What does this pull request change?

Improving error message with filename, creating a new Exception.

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-773?atlOrigin=eyJpIjoiNTBiZjFmNmVhOGE3NGExOWE4OTliZjg1ODM4NDRiNjQiLCJwIjoiaiJ9

@frodehk frodehk self-assigned this May 28, 2024
@frodehk frodehk requested a review from a team as a code owner May 28, 2024 14:26
@@ -396,7 +400,7 @@ def _validate_headers(headers: List[str]):
"[ _ - # + : . , /] "
)
elif re.match(r"^Unnamed: \d+$", header):
raise ValueError("CSV input file must include header")
raise InvalidResourceHeaderException(f"One or more headers are missing in input-file {name}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

in my opinion we should probably not have a naution about filename here. It may not always be a filename, it may be a csv dataset retrieved from another place? Therefore, a specific exception as you have here is good, and then catch that higher up, where you can add the additional information about the filename. how does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. To understand, higher up - you mean to catch it later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Basically, if we have an error reading a csv file we would probably have to "stop the line", as in stop execution in order for the user to fix the error to continue? So, yes, as late as possible, higher up the hierarchy, where we are actually trying to read a file called "test.csv", or a data stream...which in theory is also possible...close to the error you have the details of what went wrong (characters on the specific line aso.), and higher up you have the details of the greater task (read a file)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TeeeJay : The error is now catched differently. Please have a look and see if it is better.

@frodehk frodehk merged commit b146eb4 into main May 31, 2024
9 checks passed
@frodehk frodehk deleted the ECALC-773-improve-error-message-missing-header-csv branch May 31, 2024 13:34
equinor-schen pushed a commit that referenced this pull request Aug 23, 2024
)

* chore: improve error message for missing header in input csv files

ECALC-773

---------

Co-authored-by: Thomas Falch Johansen <[email protected]>
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