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

Knocking out TODO comments where possible #220

Merged
merged 10 commits into from
Jul 24, 2023
Merged

Knocking out TODO comments where possible #220

merged 10 commits into from
Jul 24, 2023

Conversation

nh916
Copy link
Contributor

@nh916 nh916 commented Jul 21, 2023

Description

Knocked out a few small TODO that was left. Not a huge improvement to the project, but helps knock out some things that we've been putting aside

Changes

  • changing TODO comment with pytest.skip
  • changed checking db schema length in test_api.py
    • instead of checking if the db schema is the exact same length, I am checking if it has more than 30 fields
    • db schema is always changing so this test will break often, but it should have more than 30 fields because there are at least 24 nodes

  • renamed complex_algorithm_node to simple_algorithm_node
    • because it was minimalistic and not maximal node with all required arguments
    • made changes across all the tests that were using it
  • changed simple_property_node fixture to named arguments, and added docstrings to it
  • cleaned up complex_process_node fixture
    • using simple fixtures for it
    • using fixtures instead of making nodes
    • avoiding deep_copy as that causes issues
  • allowing for Path object in api constructor for config file
    • removed unneeded comment
  • simple_process_node does not need deep_copy within the fixture as is more straight forward and all tests work fine without it and

Tests

Known Issues

Notes

Checklist

  • My name is on the list of contributors (CONTRIBUTORS.md) in the pull request source branch.
  • I have updated the documentation to reflect my changes.

@trunk-io
Copy link

trunk-io bot commented Jul 21, 2023

Merging to develop in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

instead of checking if the db schema is the exact same, I am checking if it has more than 30 fields

db schema is always changing so this test will break often, but it should have more than 30 fields because there are at least 24 nodes
* added docstrings
* made instantiation into named arguements
* changed tha variable name from `p` to `my_property`
* still functions exactly the same
* `complex_algorithm_node` was actually minimalistic with only required arguments instead of all arguments
* all tests are passing as before
* avoiding `deep_copy` as that causes issues
* using simple fixtures
  * to avoid deep_copy and to make working with a huge node easier
* using fixtures instead of remaking nodes
* `complex_process` fixture is not being used in any tests
@nh916 nh916 marked this pull request as ready for review July 21, 2023 22:53
@nh916 nh916 requested a review from InnocentBug July 21, 2023 22:53
simple_process_node does not need deep_copy within the fixture as is more straight forward and all tests work fine without it and
@nh916 nh916 merged commit 4f5f5a8 into develop Jul 24, 2023
14 checks passed
@nh916 nh916 deleted the knock_out_todo branch July 24, 2023 20:25
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