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

518 code list config same as field serialisation #538

Merged

Conversation

GDonRanasinghe
Copy link
Contributor

This PR includes the implementations for supporting concepts defined elsewhere when defining a code list using code list config.

Satisfied #518

@GDonRanasinghe GDonRanasinghe self-assigned this Jul 4, 2022
Copy link
Contributor

@robons robons left a comment

Choose a reason for hiding this comment

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

Looks sensible, apart from the behaviour tests which aren't clear what they're trying to test.

src/csvcubed/writers/skoscodelistwriter.py Outdated Show resolved Hide resolved
tests/behaviour/cube.feature Show resolved Hide resolved
…serted with variables instead of json object matching

Updated behave tests in cube feature so that the cols and data are asserted with variables instead of json object matching
Copy link
Contributor

@robons robons left a comment

Choose a reason for hiding this comment

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

I think it's moving in the right direction, the tests are definitely less brittle now, we just need to make sure we're testing the right things.

tests/behaviour/cube.feature Show resolved Hide resolved
]
"""

Scenario: Output a cube when an inline code list is defined using code list config schema v1.0 and when there are references to concepts defined elsewhere.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should really be asserting that the code list CSV file which has been written to disk has the same as column containing the expected values.

Copy link
Contributor Author

@GDonRanasinghe GDonRanasinghe Jul 8, 2022

Choose a reason for hiding this comment

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

@robons The cols and their content are tested in the following unit tests at test_code_list_config.py: test_code_list_config, test_code_list_config_without_schema, test_code_list_config_with_hierarchy.

tests/behaviour/cube.feature Show resolved Hide resolved
tests/behaviour/cube.feature Show resolved Hide resolved
@GDonRanasinghe
Copy link
Contributor Author

I think it's moving in the right direction, the tests are definitely less brittle now, we just need to make sure we're testing the right things.

@robons I have replied to your comments below outlining the unit tests as discussed.

@GDonRanasinghe GDonRanasinghe merged commit 59d192f into main Jul 8, 2022
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