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 fields sub-delimiter and cardinality handling #754

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

joecorall
Copy link
Contributor

@joecorall joecorall commented Mar 18, 2024

Link to Github issue or other discussion

#755

What does this PR do?

Simplifies the field definition implementations.

What changes were made?

  • The fields definitions were checking if a sub-delimiter existed in the CSV cell and calling two separate pieces of logic in the case of existing or not existing. That can be simplified by always splitting on the sub-delimiter and handling the values that way: df1df23
  • Similarly, the cardinality handling of limited vs unlimited can be simplified by handling unlimited and limited in the same if conditional: cca4d94
if -1 < cardinality < len(subvalues)
    // remove additional values over cardinality

How to test / verify this PR?

Unit tests should cover this as no new code was added, only removing if/else conditons on whether the sub-delimiter exists.

Interested Parties

@mjordan


Checklist

  • Before opening this PR, have you opened an issue explaining what you want to to do?
  • Have you included some configuration and/or CSV files useful for testing this PR?
  • Have you written unit or integration tests if applicable?
  • Does the code added in this PR require a version of Python that is higher than the current minimum version?
  • If the changes in this PR require an additional Python library, have you included it in setup.py?
  • If the changes in this PR add a new configuration option, have you provided a default for when the option is not present in the .yml file?

@joecorall joecorall changed the title Simplify fields code to always split on subdelimiter Refactor fields to always split on sub-delimiter Mar 18, 2024
@joecorall joecorall changed the title Refactor fields to always split on sub-delimiter Refactor fields sub-delimiter and cardinality handling Mar 18, 2024
@joecorall joecorall marked this pull request as ready for review March 18, 2024 18:13
@mjordan
Copy link
Owner

mjordan commented Mar 27, 2024

Thanks @joecorall!

@mjordan mjordan merged commit 514b8f3 into mjordan:main Mar 27, 2024
5 checks passed
@joecorall joecorall deleted the refactor-fields branch March 27, 2024 23:24
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