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(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) #625

Merged
merged 22 commits into from Nov 10, 2023

Conversation

jnussbaum
Copy link
Collaborator

@jnussbaum jnussbaum commented Nov 7, 2023

@jnussbaum jnussbaum self-assigned this Nov 7, 2023
@jnussbaum jnussbaum changed the title feat(excel2xml): add flags to mute warnings and print errors instead of raising them (DEV-2917) feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) Nov 8, 2023
@dasch-swiss dasch-swiss deleted a comment from linear bot Nov 8, 2023
@jnussbaum jnussbaum changed the title feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings Nov 8, 2023
@jnussbaum jnussbaum changed the title feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) Nov 8, 2023
@jnussbaum jnussbaum changed the title feat(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) chore(excel2xml): suppress irrelevant warnings, turn non-fatal errors into warnings (DEV-2917) Nov 9, 2023
Copy link
Collaborator

@Nora-Olivia-Ammann Nora-Olivia-Ammann left a comment

Choose a reason for hiding this comment

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

From what I see here is that with this structure we create XMLs based on excels that are missing mandatory values. Why would this be desirable?

src/dsp_tools/excel2xml.py Outdated Show resolved Hide resolved
src/dsp_tools/excel2xml.py Show resolved Hide resolved
src/dsp_tools/excel2xml.py Show resolved Hide resolved
@jnussbaum
Copy link
Collaborator Author

From what I see here is that with this structure we create XMLs based on excels that are missing mandatory values. Why would this be desirable?

This was not the intention. Rather, I want that the entire program runs through and gives me a list of warnings/errors. Previously, the first error interrupted the program, so the user never knew the entire list of problems. The idea behind this PR is that creating an invalid XML is acceptable, because the user has then a list of problems, so that they can fix them directly in the XML, or they can fix it in the Excel and then run the command again.

This PR was born out of a real-world-scenario: I exported dokubib from salsah, converted it to XML, and sent the corrupt XML with the problem list to Rita, who takes care about fixing the problems.

@Nora-Olivia-Ammann
Copy link
Collaborator

From what I see here is that with this structure we create XMLs based on excels that are missing mandatory values. Why would this be desirable?

This was not the intention. Rather, I want that the entire program runs through and gives me a list of warnings/errors. Previously, the first error interrupted the program, so the user never knew the entire list of problems. The idea behind this PR is that creating an invalid XML is acceptable, because the user has then a list of problems, so that they can fix them directly in the XML, or they can fix it in the Excel and then run the command again.

This PR was born out of a real-world-scenario: I exported dokubib from salsah, converted it to XML, and sent the corrupt XML with the problem list to Rita, who takes care about fixing the problems.

Could we not set a flag so that we can stop the program in the end if any error occurred?

@jnussbaum jnussbaum merged commit b6943a2 into main Nov 10, 2023
6 checks passed
@jnussbaum jnussbaum deleted the wip/dev-2917-mute-errors branch November 10, 2023 10:21
@daschbot daschbot mentioned this pull request Nov 10, 2023
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.

None yet

2 participants