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(floorist): RHICOMPL-2984 Bugfix for empty prefix floorplan #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Victoremepunto
Copy link
Collaborator

@Victoremepunto Victoremepunto commented May 24, 2022

  • Fix a bug for when floorplan file does not provide prefix
  • Add helper function for generating directory names
  • Add helper function for validating Floorplan files
  • Add unit test coverage

@Victoremepunto Victoremepunto force-pushed the empty-prefix-is-not-handled branch from 01e2279 to 1d3d282 Compare May 24, 2022 23:17
@Victoremepunto Victoremepunto changed the title Bugfix and add test coverage fix(floorist): RHICOMPL-2984 Bugfix for empty prefix floorplan May 24, 2022
@Victoremepunto Victoremepunto requested a review from skateman May 24, 2022 23:20
@Victoremepunto Victoremepunto force-pushed the empty-prefix-is-not-handled branch 2 times, most recently from 6dd9d5e to 4cd5c78 Compare May 24, 2022 23:32
@Victoremepunto Victoremepunto force-pushed the empty-prefix-is-not-handled branch 4 times, most recently from a0049ab to f028b1c Compare June 3, 2022 16:37
 - Fix a bug for when floorplan file does not provide prefix
 - Add helper function for generating directory names
 - Add helper function for validating Floorplan files
 - Add unit test coverage
@Victoremepunto Victoremepunto force-pushed the empty-prefix-is-not-handled branch from f028b1c to 1dc01c9 Compare June 3, 2022 18:13
@skateman
Copy link
Member

@Victoremepunto the PR is providing a bit more than it was described in the epic. Can we reduce it to a minimal solution and extract & discuss the unit testing + refactoring in a separate ticket please?

@Victoremepunto
Copy link
Collaborator Author

@Victoremepunto the PR is providing a bit more than it was described in the epic. Can we reduce it to a minimal solution and extract & discuss the unit testing + refactoring in a separate ticket please?

@skateman what is the minimal part you'd like to see out of it ? I created the tests before the feature was implemented and I see them as a whole. What is being added is:

  • the fix (the changes in "floorist.py" and because of the refactor I had to do , the "helpers" module.
  • unit tests for this patch and for the helpers.
  • an integration test for the same feature.

@skateman
Copy link
Member

skateman commented Aug 3, 2022

I am still conservative about unit tests here and it feels like you're doing the refactor in order to be able to do unit tests. The scope of the project is still small enough to have just high-level tests and as long as it is so, I would like to keep it like this. Again, in case of a go-reimplementation we can just drop-in replace the main() calls in these.

@Victoremepunto
Copy link
Collaborator Author

I am still conservative about unit tests here and it feels like you're doing the refactor in order to be able to do unit tests. The scope of the project is still small enough to have just high-level tests and as long as it is so, I would like to keep it like this. Again, in case of a go-reimplementation we can just drop-in replace the main() calls in these.

Yes, exactly. I had to refactor because the way it is implemented is impossible to test. I don't think that's a desirable situation and decided to fix it.

Please, suggest the next course of action

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