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

fix: excel2properties no longer crashes if optional columns are missing (DEV-3468) #907

Conversation

Nora-Olivia-Ammann
Copy link
Collaborator

No description provided.

@Nora-Olivia-Ammann Nora-Olivia-Ammann self-assigned this Apr 9, 2024
Copy link

linear bot commented Apr 9, 2024

Copy link
Collaborator

@jnussbaum jnussbaum left a comment

Choose a reason for hiding this comment

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

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

src/dsp_tools/commands/excel2json/properties.py Outdated Show resolved Hide resolved
@jnussbaum
Copy link
Collaborator

Concerning the PR title: "no longer crashes if optional columns are missing" is a sentence without subject, which is a bit fishy. If I read a changelog, I want to read a list of actions that were taken, e.g. "prevent crash if optional columns are missing"

@Nora-Olivia-Ammann
Copy link
Collaborator Author

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

What would you like to have tested?

@jnussbaum
Copy link
Collaborator

jnussbaum commented Apr 9, 2024

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

What would you like to have tested?

Currently, there is no test that prevents us from accidentally fall back into a worse state than we have now (=regression). Ideally, you would write a test that fails on the main branch, but succeeds on this branch here.

@Nora-Olivia-Ammann
Copy link
Collaborator Author

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

What would you like to have tested?

Currently, there is no test that prevents us from accidentally fall back into a worse state than we have now (=regression). Ideally, you would write a test that fails on the main branch, but succeeds on this branch here.

Right I can do that.

@Nora-Olivia-Ammann Nora-Olivia-Ammann changed the title fix(excel2properties): no longer crashes if optional columns are missing (DEV-3468) fix: excel2properties no longer crashes if optional columns are missing (DEV-3468) Apr 9, 2024
@Nora-Olivia-Ammann
Copy link
Collaborator Author

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

What would you like to have tested?

Currently, there is no test that prevents us from accidentally fall back into a worse state than we have now (=regression). Ideally, you would write a test that fails on the main branch, but succeeds on this branch here.

Right I can do that.

I just checked, we do test that and the test never failed. So further confusion abounds, but I am 99% sure we did not make it worse...

@Nora-Olivia-Ammann Nora-Olivia-Ammann merged commit 523d55b into main Apr 9, 2024
10 checks passed
@Nora-Olivia-Ammann Nora-Olivia-Ammann deleted the wip/dev-3468-fix-excel2property-crash-when-missing-optional-columns branch April 9, 2024 09:08
@daschbot daschbot mentioned this pull request Apr 9, 2024
@jnussbaum
Copy link
Collaborator

LGTM. Would it be worth the effort to add a test/test file to prevent regression?

What would you like to have tested?

Currently, there is no test that prevents us from accidentally fall back into a worse state than we have now (=regression). Ideally, you would write a test that fails on the main branch, but succeeds on this branch here.

Right I can do that.

I just checked, we do test that and the test never failed. So further confusion abounds, but I am 99% sure we did not make it worse...

Ui, that doesn't sound good. This should be investigated. But apart from that, I rather thought about preventing future regression. It can easily happen in some months that someone changes the code and accidentally introduces a new bug.

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