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

Copy Python tests for Modulemd.ModuleStream.documentation into C #398

Merged
merged 4 commits into from
Dec 9, 2019
Merged

Conversation

srikavin
Copy link
Contributor

@srikavin srikavin commented Dec 4, 2019

This copies the Python test test_documentation into C. This is related to #199.

Copy link
Member

@OrionStar25 OrionStar25 left a comment

Choose a reason for hiding this comment

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

CI is failing because of linting issue. Code LGTM though.

Copy link
Collaborator

@sgallagher sgallagher left a comment

Choose a reason for hiding this comment

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

It looks mostly good. I'd like you to do two things:

  • You have "http://redhat.com" and "documentation" copied-and-pasted in many places. I'd rather see you use #define MMD_TEST_DOC_TEXT "http://redhat.com" and #define MMD_TEST_DOC_PROP "documentation" instead, so if we ever want to change the values, we only need to change one place.
  • I'd like you to store and get a string for the documentation value that includes non-ASCII characters such as:
    • À
    • ϶
    • 🌭

I realize the unicode requirement wasn't in the python version of the test, but that was an oversight on my part. Please correct that while you're in this code.

Fixes a possible double free as get_documentation does not transfer ownership
@srikavin
Copy link
Contributor Author

srikavin commented Dec 5, 2019

@sgallagher, I've replaced the hard-coded values with defines and added a Unicode test to both the C and Python tests.

Copy link
Collaborator

@sgallagher sgallagher left a comment

Choose a reason for hiding this comment

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

Looks good!

@sgallagher sgallagher merged commit 257cf50 into fedora-modularity:master Dec 9, 2019
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.

3 participants