Feature: HTTP `DELETE` node #293

nh916 · 2023-08-25T22:25:57Z

Description

giving users a way to delete a nodes from the API

Changes

Wrote Delete

wrote cript.API.delete() method
wrote integration tests for delete
wrote full documentation for delete

Updated Logger

created a set_log() method
- it essentially sets the logger for the whole cript.API class
  - was considering making it into a getter that would create a new logger and return it, but since each class should only have one logger that might encourage bad programming and create bugs
- made verbose into a property that changes the class logger after the user sets it.
  - The user simply sets it casually and we rearrange the logger behind the scenes for them to work correctly so they can turn verbose ON and OFF easily throughout their script
  - The logger has good simple logic so we don't have to keep putting if self.verbose then log
- I am not sure if the way I am doing logger is perfect right now, but I think it is good for our use cases for now and we can upgrade it as we go
wrote/updated documentation for cript.API.verbose and the logging in general for that class

Updated Paginator Errors

changed paginator exception from Exception to APIError
upgraded/refactored APIError
- APIError did not have as much information and was unhelpful in debugging
- gave it more information and made it much more helpful for users to read and for us to debug

Updated Documentation

updated documentation: noticed that documentation for cript.API tells users to use host as criptapp.org and since that was outdated, I changed it to api.criptapp.org

Tests

wrote out integrate_delete_node_helper

Screenshots

Terminal Log: `cript.API.delete()` Successfully Deleted a Node

This is logged to the terminal after the SDK successfully deletes a node from the API

New APIError Exception Terminal Output:

Documentation: `cript.API.delete()`

Documentation: `cript.API logging`

Known Issues

We are only using logger in the cript.API but if we start putting it in more places then we'll need a refactor for it to make it cleaner and consider putting it within cript/logger.py

Notes

Integration Tests

Recommendation: I recommend running the all SDK tests locally to confirm everything I did works correctly

click to see screenshots of all tests ran locally

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.

* wrote basic delete method * not tested yet * wrote basic refresh * not tested yet * wrote test for refresh * but not finished yet

trunk-io · 2023-08-25T22:25:59Z

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.

nh916 · 2023-08-25T22:30:11Z

I wrote delete before here: #255

I'll be putting that logic into this branch and much more. Then I'll delete the old branch.

mypy says that the `logger: logging.Logger = None` this cannot be None because you said it should be `logging.Logger` but before it is initialized it would be None. Don't want to type it as `Optional` because it should always exist and it is not Optional, so just ignoring the error for now.

will add in later if needed

* wrote out logic and docstrings for `integrate_delete_node_helper` * wrote notes for future upgrades * ready to write tests for every node

* wrote out full test function for `delete_integration_node_helper` in integration_test_helper.py * cleaned up test_project.py test code > wrote a small snippet for my printing JSON preference

… a package

nh916 · 2023-08-30T00:06:40Z

@InnocentBug any ideas on what is making CI unhappy?

it seems to think that Optional is not imported into src/cript/api/exceptions.py but it is imported and when I run mypy locally it runs successfully.

InnocentBug · 2023-08-30T00:37:05Z

@InnocentBug any ideas on what is making CI unhappy?

it seems to think that Optional is not imported into src/cript/api/exceptions.py but it is imported and when I run mypy locally it runs successfully.

Yeah, weird one.
No idea where it comes from.

I got it happy by upgrading trunk #301

nh916 · 2023-08-30T01:29:16Z

I was originally in a bit of a dilemma when first writing this but I think to keep the whole documentation consistent I will keep Numpy style for the warnings. Since I already wrote out the whole post, I'll post it anyways so we are all aware.

click to expand

SDK Warning

The warning could look a bit better if we decide to go with either material mkdocs admonitions or change the whole project to use to Google style docstrings as MKDocstrings seems to parse Google style docstrings style better.

I have left a discussion post on MKDocstrings repo better parsing Numpy style, but I don't think much came of it

Numpy Style

Numpy documentation way of writing a warning is like this

Docstrings Code

"""
Warnings
--------
After successfully deleting a node from the API, keep in mind that your local Project node in your script
may still contain outdated data as it has not been synced with the API.

To ensure you have the latest data, follow these steps:

1. Fetch the newest Project node from the API using the [`cript.API.search()`](./#cript.api.api.API.search) provided by the SDK.
1. Deserialize the retrieved data into a new Project node using the [`load_nodes_from_json`](../../utility_functions/#cript.nodes.util.load_nodes_from_json) utility function.
1. Replace your old Project node with the new one in your script for accurate and up-to-date information.
"""

Documentation Output

IDE Documentation Output

Material MKDocs Style

However, we can use material mkdocs admonitions to style it up a bit and make it look better

Docstrings Code

"""
!!! Warning "Refresh Project Node After Deletion"
	After successfully deleting a node from the API, keep in mind that your local Project node in your script
	may still contain outdated data as it has not been synced with the API.

	To ensure you have the latest data, follow these steps:

	1. Fetch the newest Project node from the API using the [`cript.API.search()`](./#cript.api.api.API.search) provided by the SDK.
	1. Deserialize the retrieved data into a new Project node using the [`load_nodes_from_json`](../../utility_functions/#cript.nodes.util.load_nodes_from_json) utility function.
	1. Replace your old Project node with the new one in your script for accurate and up-to-date information.
"""

Documentation Output

IDE Documentation Output

added example code and example log

nh916 · 2023-08-31T20:15:10Z

This is a bit of a big PR. I apologize for that.

This could be broken down into smaller PRs if we need, but in the interest of making our lives easy and not having merge conflicts I think we should keep it as one PR

added comments to code for clarity

InnocentBug

I have some minor requests.
Plus I think coverage is failing now.

src/cript/api/exceptions.py

InnocentBug · 2023-08-31T23:05:29Z

tests/integration_test_helper.py

+        return
+
+    # check that the node we want to delete first exists on the API
+    my_node_paginator = cript_api.search(


I understand the logic.
But would it be better to just send delete and handle the error from API if the node doesn't exist?

that could streamline the process, but the only thing that we would lose from that would be that we did not prove that the node was there before we went to delete it, so if we start getting bugs we might be asking ourselves,

"is it having issues because the API can't delete the node? or is it because the node never existed on the API?"

I was thinking that writing it this way could be a bit more explicit, but removing the first search would honestly not take away much as far as I can see and would be pretty easy too.

You think remove it?

I think, I would insist on getting rid of it in production code, but this is tests only and should be OK.

I would honestly be happy getting rid of it too either now or in a future PR. I think both should work and be okay

nh916 added 2 commits August 25, 2023 12:39

started working on delete

6a6ca71

* wrote basic delete method * not tested yet * wrote basic refresh * not tested yet * wrote test for refresh * but not finished yet

setup _set_logger method for cript.API

c4d0d3f

added Notes to delete docstrings

688e296

nh916 changed the title ~~Feature delete~~ Feature: HTTP DELETE node Aug 25, 2023

refactored logger and verbose

125eedd

nh916 added the enhancement New feature or request label Aug 25, 2023

nh916 added 22 commits August 25, 2023 16:05

removed unused imports and formatted with black

1531c0c

formatting with black and removing unused imports

0e910ab

removing cript.API.refresh() method for now

53d7c36

will add in later if needed

format with trunk

cb4480d

changed logging format of cript.API.delete

421ddc7

changed the error for paginator from generic Exception to APIError

0ae36d8

wrote DELETE for test_project.py

0498d84

wrote out logic for integrate_delete_node_helper

6f6d6bd

* wrote out logic and docstrings for `integrate_delete_node_helper` * wrote notes for future upgrades * ready to write tests for every node

updated test_project.py comment

2541d1e

added type hinting to paginator.py response

df43358

wrote out delete_integration_node_helper

a2edf83

* wrote out full test function for `delete_integration_node_helper` in integration_test_helper.py * cleaned up test_project.py test code > wrote a small snippet for my printing JSON preference

updated comments

791b473

updated test_integration_collection

5f3f50c

wrote Delete integration test for collection

14c4692

wrote Delete integration test for experiment

1e38b22

wrote Delete integration test for process

e261ed4

wrote Delete integration test for computation

a42d2d0

fixed delete integration function imports for tests since it is not…

2c545d8

… a package

wrote Delete integration test for data

69cc8a2

wrote Delete integration test for inventory

7ae4cfe

wrote Delete integration test for file

980d5b8

nh916 added 3 commits August 29, 2023 16:51

formatted with trunk

9dc819e

working on fixing CI issues

b6fcdf3

optimizing imports

37519a1

nh916 and others added 2 commits August 29, 2023 17:19

upgraded documentation for cript.API.delete()

f5c8cb6

trunk fmt

5349cb2

nh916 added 4 commits August 29, 2023 18:43

updated cript.API.delete() documentation

1cdfb21

updated host documentation in cript.API

95f0399

upgraded documentation for cript.API.verobose

7857e7b

added example code and example log

fixed BIGSMILES search test

eb47dd2

nh916 added 4 commits August 31, 2023 13:41

removed Navid log style from integration_test_helper.py

9cefd2e

added comments to code for clarity

Merge branch 'develop' into feature-delete

998dfcd

Merge branch 'develop' into feature-delete

5fcdef5

formatted with trunk

7b2e0cb

nh916 marked this pull request as ready for review August 31, 2023 21:03

nh916 added 5 commits August 31, 2023 14:09

importing Optional to cript/api/exceptions.py

22eb336

formatted with trunk

caea71f

added timeout to requests.delete()

3544320

fixed import error

3137cfd

formatted with trunk

4b4ae7c

nh916 requested a review from InnocentBug August 31, 2023 21:22

InnocentBug requested changes Aug 31, 2023

View reviewed changes

nh916 and others added 2 commits September 1, 2023 10:51

Merge branch 'develop' into feature-delete

96bc812

making trunk happy

c1e3395

InnocentBug approved these changes Sep 1, 2023

View reviewed changes

nh916 merged commit 8df4f1b into develop Sep 1, 2023
14 checks passed

nh916 deleted the feature-delete branch September 1, 2023 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: HTTP `DELETE` node #293

Feature: HTTP `DELETE` node #293

nh916 commented Aug 25, 2023 •

edited

Loading

trunk-io bot commented Aug 25, 2023

nh916 commented Aug 25, 2023

nh916 commented Aug 30, 2023

InnocentBug commented Aug 30, 2023

nh916 commented Aug 30, 2023 •

edited

Loading

SDK Warning

Numpy Style

Docstrings Code

Documentation Output

IDE Documentation Output

Material MKDocs Style

Docstrings Code

Documentation Output

IDE Documentation Output

nh916 commented Aug 31, 2023

InnocentBug left a comment

InnocentBug Aug 31, 2023

nh916 Aug 31, 2023

InnocentBug Sep 1, 2023

nh916 Sep 1, 2023

Feature: HTTP DELETE node #293

Feature: HTTP DELETE node #293

Conversation

nh916 commented Aug 25, 2023 • edited Loading

Description

Changes

Wrote Delete

Updated Logger

Updated Paginator Errors

Updated Documentation

Tests

Screenshots

Terminal Log: cript.API.delete() Successfully Deleted a Node

New APIError Exception Terminal Output:

Documentation: cript.API.delete()

Documentation: cript.API logging

Known Issues

Notes

Integration Tests

Checklist

trunk-io bot commented Aug 25, 2023

nh916 commented Aug 25, 2023

nh916 commented Aug 30, 2023

InnocentBug commented Aug 30, 2023

nh916 commented Aug 30, 2023 • edited Loading

SDK Warning

Numpy Style

Docstrings Code

Documentation Output

IDE Documentation Output

Material MKDocs Style

Docstrings Code

Documentation Output

IDE Documentation Output

nh916 commented Aug 31, 2023

InnocentBug left a comment

Choose a reason for hiding this comment

InnocentBug Aug 31, 2023

Choose a reason for hiding this comment

nh916 Aug 31, 2023

Choose a reason for hiding this comment

InnocentBug Sep 1, 2023

Choose a reason for hiding this comment

nh916 Sep 1, 2023

Choose a reason for hiding this comment

Feature: HTTP `DELETE` node #293

Feature: HTTP `DELETE` node #293

nh916 commented Aug 25, 2023 •

edited

Loading

Terminal Log: `cript.API.delete()` Successfully Deleted a Node

Documentation: `cript.API.delete()`

Documentation: `cript.API logging`

nh916 commented Aug 30, 2023 •

edited

Loading