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

Formalize the Dioptra Client #220

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Conversation

alexb1200
Copy link
Contributor

Towards Resolving #210

@jkglasbrenner
Copy link
Collaborator

Preparing to review: I split off the changes under the examples/ folder to another branch, https://github.com/usnistgov/dioptra/tree/examples-use-moved-dioptra-client. I am also squashing and rebasing the rest of the commits on the dev branch to make sure all of the tests continue to pass.

@jkglasbrenner jkglasbrenner force-pushed the abyrne-formalize-dioptra-client branch from 8109ec9 to a7bb7ac Compare July 27, 2023 16:52
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.

Most of this review is focused on the whitespace structure of the docstrings themselves. Basically, although there are 64 comments, a lot of them are duplicative and are just there to flag the places I noticed something to be addressed.

The change requests are mainly about how to update the docstrings so they conform with the 88 characters or less per line style rule and ensuring there are 4 spaces per indentation level. There's also suggestions on how to structure the whitespace for the responses in the example blocks so they're following a consistent style everywhere. The rest are just minor things like deleting extra lines of whitespace.

src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
src/dioptra/client/_client.py Show resolved Hide resolved
src/dioptra/client/_client.py Outdated Show resolved Hide resolved
@jkglasbrenner jkglasbrenner force-pushed the abyrne-formalize-dioptra-client branch 3 times, most recently from 013e985 to 4745ab8 Compare August 18, 2023 21:42
This update formalizes the `DioptraClient` class in examples/scripts/client.py by adding it to the
dioptra package and adding documentation for it.
@jkglasbrenner jkglasbrenner force-pushed the abyrne-formalize-dioptra-client branch from 4745ab8 to 3505f6f Compare August 18, 2023 21:44
@jkglasbrenner jkglasbrenner merged commit d532c79 into dev Aug 18, 2023
13 of 19 checks passed
@jkglasbrenner jkglasbrenner deleted the abyrne-formalize-dioptra-client branch August 18, 2023 21:49
@jkglasbrenner jkglasbrenner added the refactor Changes that neither fix a bug nor add a feature label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes that neither fix a bug nor add a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants