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.

Sign up for GitHub

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

Jump to bottom

code cleanup #98

Merged
merged 2 commits into from
Oct 13, 2024
Merged

code cleanup #98

merged 2 commits into from
Oct 13, 2024

Conversation

a-thieme
Copy link
Contributor

Lots of code cleanup:

  • change '== None' to 'is None' and '!= None' to 'is not None'
  • simplify compound conditionals
  • remove redundant parenthesis
  • spelling/grammar
  • rename loop variable to not shadow existing variables
  • remove unneeded imports
  • change imported types into builtin
  • merge duplicated code from command handles into a function in the command handle base class
  • update doxygen docs
  • add "fixme:" where errors should occur but don't
  • add "fixme:" for unused function parameters
  • remove unused variables
  • rename camelCase variables
  • remove 'self' and make functions static if they are static
  • add mongodb and leveldb to the integration test configuration
  • integration test now uses leveldb (we already have tests for sqlite3 in tests/storage_test.py, so this will just provide more coverage)

Adam Thieme and others added 2 commits October 12, 2024 20:47
code cleanup
195ec67
@a-thieme
Revert leveldb to sqlite3 in integration_test.py
1e3ec0b 
GitHub actions don't seem to import leveldb things correctly
@a-thieme
Copy link
Contributor Author

is LevelDB not supported with the current automatic tests? It seems to have not imported the LevelDBStorage class for some reason.

zjkmxy
zjkmxy approved these changes Oct 13, 2024
View reviewed changes
ndn_python_repo/utils/pubsub.py
@@ -190,6 +190,7 @@ async def _process_notify_interest(self, int_name, int_param, app_param):
"""
Async helper for ``_on_notify_interest()``.
"""
# fixme: why isn't int_param used?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which parameter is related?

@zjkmxy
Copy link
Collaborator

zjkmxy commented Oct 13, 2024

is LevelDB not supported with the current automatic tests? It seems to have not imported the LevelDBStorage class for some reason.

I remember it failed to work. You can try to enable it and see if it works.

@a-thieme
Copy link
Contributor Author

is LevelDB not supported with the current automatic tests? It seems to have not imported the LevelDBStorage class for some reason.

I remember it failed to work. You can try to enable it and see if it works.

My first commit just changed the integration test to use leveldb instead of sqlite3 and it failed the tests. the second commit fixed that

@zjkmxy
Copy link
Collaborator

zjkmxy commented Oct 13, 2024

is LevelDB not supported with the current automatic tests? It seems to have not imported the LevelDBStorage class for some reason.

I remember it failed to work. You can try to enable it and see if it works.

My first commit just changed the integration test to use leveldb instead of sqlite3 and it failed the tests. the second commit fixed that

https://github.com/UCLA-IRL/ndn-python-repo/actions/runs/11311234689/job/31457225541#step:7:91

I guess it is not even installed on CI.

@zjkmxy zjkmxy merged commit a22c253 into UCLA-IRL:master Oct 13, 2024
2 checks passed
@a-thieme a-thieme deleted the code-cleanup branch October 13, 2024 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zjkmxy zjkmxy zjkmxy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a-thieme @zjkmxy