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

Address Issue #74 - [DOC] add typings to functions #81

Merged
merged 5 commits into from
Aug 30, 2024

Conversation

michael-linnane-lrn
Copy link
Contributor

@michael-linnane-lrn michael-linnane-lrn commented Aug 16, 2024

Problem Statement

Open Issues this addresses: https://github.com/Learnosity/learnosity-sdk-python/issues/74
The functions used in this package do not have any type hints included. This PR addresses that.

PR Actions

closes #74

What was done

  • Added type and return hints to all functions
  • Added new unit tests for daatapi.py to improve coverage
  • Fixed usage of deprecated iterritems

How was this change tested

  • Ran mypy --strict learnosity_sdk to verify all type hints and issues
  • Ran test suite
  • Ran coverage checker
  • Ran learnosity-sdk-assessment-quickstart with local build

Notes and TODOs

  • There are usages of default arguments being set to {}. This is a dangerous action and should be addressed in another PR
  • Tests added
  • All testsuites passed
  • make dist completed successfully

@michael-linnane-lrn michael-linnane-lrn linked an issue Aug 16, 2024 that may be closed by this pull request
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 praise: Nice work! 🚀 Be great to finally have full typing!

👏 praise: Great and clear PR description as well! Really helped with context for reviewing!

🎯 suggestion (non-blocking): maybe if there's an appetite for this we can add mypy --strict to the pipeline to ensure we maintain our current typing after all this effort! This could maybe be a follow up PR if the mainters are happy?

tests/unit/test_dataapi.py Outdated Show resolved Hide resolved
learnosity_sdk/request/init.py Outdated Show resolved Hide resolved
Copy link
Contributor

@shtrom shtrom left a comment

Choose a reason for hiding this comment

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

Looking good! Couple of notes on the release process and Codacy warnings.

ChangeLog.md Outdated Show resolved Hide resolved
learnosity_sdk/_version.py Show resolved Hide resolved
learnosity_sdk/request/dataapi.py Show resolved Hide resolved
@michael-linnane-lrn michael-linnane-lrn changed the title Address Issue #74 - add typings to functions Address Issue #74 - [DOC] add typings to functions Aug 21, 2024
@michael-linnane-lrn michael-linnane-lrn self-assigned this Aug 23, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

🚢 🦜

ChangeLog.md Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@michael-linnane-lrn michael-linnane-lrn merged commit e10dfeb into master Aug 30, 2024
1 of 3 checks passed
@michael-linnane-lrn michael-linnane-lrn deleted the ml-add-typings branch August 30, 2024 14:36
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.

Type stubs
3 participants