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 "mergeAreaRecords" utility #225

Open
kettanaito opened this issue Sep 18, 2019 · 2 comments
Open

Refactor "mergeAreaRecords" utility #225

kettanaito opened this issue Sep 18, 2019 · 2 comments
Labels
enhancement New feature or request priority:low scope:calculation This issue relates to the calculation logic

Comments

@kettanaito
Copy link
Owner

This is a follow up to #208

What:

Need to refactor the src/utils/breakpoints/mergeAreaRecords utility function.

Why:

  • It's too smart due to accepting the includesArea flag
  • It shouldn't manage the next behavior value
  • It should be a simple A + B breakpoints merge, to be used on any two breakpoints in general (now operates on AreaRecord)
@kettanaito kettanaito added enhancement New feature or request scope:calculation This issue relates to the calculation logic priority:low labels Sep 18, 2019
@kettanaito kettanaito added this to the 0.10.0 milestone Sep 18, 2019
@ruhaise
Copy link
Collaborator

ruhaise commented Oct 16, 2019

@kettanaito some more info about this bug would be nice :)

@kettanaito
Copy link
Owner Author

Hi, @ruhaise. It's not a bug, but a refactoring of the existing code.

As I said, the function that should merge two area records is too smart. It doesn't have to know if the area is included in the template (represented by includesArea argument), because there is no template at the time of merging. It's used at the moment only to produce the next breakpoint behavior, which I also think should not be this function's concern.

In practice this task means to simplify mergeAreaRecords(), its corresponding unit test, any related integration tests that may use that function, and the usage surface. Since it will no longer accept includesArea, and won't produce next behavior, its call signature will change and would need to be adjusted where it's being used.

Overall, I think it's not a beginner-friendly task, but I don't mean to discorauge you: the library is still quite straightforward, so you will be able to understand the code if you give it a read. Feel free to ping me regarding any questions about this refactoring. Thanks!

@kettanaito kettanaito removed this from the 0.10.0 milestone Oct 25, 2019
@kettanaito kettanaito added this to the 0.11.0 milestone Nov 14, 2019
@kettanaito kettanaito removed this from the 0.11.0 milestone Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:low scope:calculation This issue relates to the calculation logic
Projects
None yet
Development

No branches or pull requests

2 participants