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

Applied general fixes from PR 667 #720

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Nov 26, 2023

Description

Applied general improvements from PR 667.

The purpose of this PR is to ensure that the fixes in PR 667 are not lost. This includes

  • save test output in a separate directory (for some tests, see issue Move test input/output to separate directories #609)
  • do not run test_property_mappings if rdflib is missing
  • documented environment variable DLITE_ATEXIT_FREE
  • fixed memory leak in test_bson_storage

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

@@ -60,7 +60,7 @@ class TransformationStatus(BaseModel):
assert inst.startTime == int(now - 3000)
utc = timezone(timedelta(hours=0))
dt = datetime.fromtimestamp(now - 600).astimezone(utc)
assert inst.finishTime == str(dt)
assert inst.finishTime.split("+")[0] == str(dt).split("+")[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please comment why

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Describe the purpose of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

comment why

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid memory leak by calling fu_endmatch() when we are done with the iterator.

To not polute the code with unnecessary comments, I prefer not to add this comment to the code since it is already documented in fu_startmatch() that the returned iterator must be cleaned up with fu_endmatch().

Copy link
Collaborator

Choose a reason for hiding this comment

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

comment on in commit

Copy link
Collaborator Author

@jesper-friis jesper-friis Dec 2, 2023

Choose a reason for hiding this comment

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

Changed name of variable from "state" to "globals" to be consistent with variable naming when get_dlite_storage_plugin_api() is called in the rest of the code.

I also think this comment belongs better here than in the code.

Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

OK. We disagree a bit on where comments should be. I guess that is because I am not enough expert in C to understand the code by itself.

@jesper-friis jesper-friis merged commit f23ddf8 into master Dec 6, 2023
11 checks passed
@jesper-friis jesper-friis deleted the general-fixes-from-pr-667 branch December 6, 2023 17:42
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