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

Refactor Experiment endpoint and remove wtforms dependency #328

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

alexb1200
Copy link
Contributor

Removed wtforms forms and corresponding models. Made minimal changes to unit tests.

@jkglasbrenner
Copy link
Collaborator

@alexb1200 The refactored experiment tests in #322 have been merged to dev. There's a branch conflict because the old experiment unit test files were deleted and this branch makes edits to them. I'll resolve these conflicts and get this branch rebased so we can confirm if your refactor passes the new tests.

@jkglasbrenner jkglasbrenner force-pushed the abyrne-experiemnts-remove-wtforms-refactor branch from 2d1d542 to 1e1b5ab Compare November 30, 2023 20:14
@jkglasbrenner
Copy link
Collaborator

Since this update changes the POST requests to no longer use forms, there are a couple places you'll need to update the Python client(s) (one being the deprecated client under examples/scripts/client.py). The first place is this:

response = requests.post(
self.experiment_endpoint,
data=experiment_registration_form,
)

The second place is this:

response = requests.post(
self.experiment_endpoint,
data=experiment_registration_form,
)

Both need the following change:

        response = requests.post(
            self.experiment_endpoint,
-           data=experiment_registration_form,
+           json=experiment_registration_form,
        )

Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

There's a couple of small changes that are needed in schema.py to ensure that the Swagger documentation looks right. In addition, a couple of small changes are needed in the Python client, see the comment I left that points out what to change.

Outside of the above, this looks good. The changes are passing on the new integration tests, so once these changes are in place, I think we'll be good to merge.

src/dioptra/restapi/experiment/schema.py Show resolved Hide resolved
src/dioptra/restapi/experiment/schema.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jkglasbrenner jkglasbrenner merged commit 491d324 into dev Dec 13, 2023
23 checks passed
@jkglasbrenner jkglasbrenner deleted the abyrne-experiemnts-remove-wtforms-refactor branch December 13, 2023 18:39
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