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

Python: remove Verify and ./diagnostics #287

Merged
merged 13 commits into from
Apr 5, 2023

Conversation

jjhenkel
Copy link

@jjhenkel jjhenkel commented Apr 3, 2023

Motivation and Context

In an effort to make the Python port more idiomatic, let's remove the Verify class and the ./diagnostics sub-module. There are many more follow-on tasks to do here, but this is a good start (and already a large enough change).

Description

This PR does the following:

  1. Removes the Verify class, and re-writes all instances of Verify.*
  2. Adds a validation.py in the ./utils sub-module to hand some of the more complex cases from Verify (checking that skills/functions/and function params have valid names)
  3. Removes the rest of the ./diagnostics sub-module (w/ a longer-term goal of removing all/most of our custom exception classes and, instead, using appropriate built-in error classes)

Contribution Checklist

For Python, I've run make pre-commit to verify style/linting; I've run pytest tests/. No tests added here, this is a cosmetic/style change.

image

@jjhenkel
Copy link
Author

jjhenkel commented Apr 3, 2023

One more small thing, just had to merge and test_config.py was out of compliance w/ make pre-commit. Brought it back into compliance, we should re-factor and standardize how we test skills (defined as files). (But that can come in a separate PR.)

@adrianwyatt adrianwyatt added the python Pull requests for the Python Semantic Kernel label Apr 3, 2023
@mkarle
Copy link
Contributor

mkarle commented Apr 4, 2023

Looks good to me!

@dluc dluc merged commit f7c8b10 into microsoft:python-preview Apr 5, 2023
dluc pushed a commit that referenced this pull request Apr 12, 2023
### Motivation and Context

In an effort to make the Python port more idiomatic, let's remove the
`Verify` class and the `./diagnostics` sub-module. There are many more
follow-on tasks to do here, but this is a good start (and already a
large enough change).

### Description

This PR does the following:

1. Removes the `Verify` class, and re-writes all instances of `Verify.*`
2. Adds a `validation.py` in the `./utils` sub-module to hand some of
the more complex cases from `Verify` (checking that skills/functions/and
function params have valid names)
3. Removes the rest of the `./diagnostics` sub-module (w/ a longer-term
goal of removing all/most of our custom exception classes and, instead,
using appropriate built-in error classes)
dluc pushed a commit that referenced this pull request Apr 13, 2023
### Motivation and Context

In an effort to make the Python port more idiomatic, let's remove the
`Verify` class and the `./diagnostics` sub-module. There are many more
follow-on tasks to do here, but this is a good start (and already a
large enough change).

### Description

This PR does the following:

1. Removes the `Verify` class, and re-writes all instances of `Verify.*`
2. Adds a `validation.py` in the `./utils` sub-module to hand some of
the more complex cases from `Verify` (checking that skills/functions/and
function params have valid names)
3. Removes the rest of the `./diagnostics` sub-module (w/ a longer-term
goal of removing all/most of our custom exception classes and, instead,
using appropriate built-in error classes)
dluc pushed a commit that referenced this pull request Apr 13, 2023
### Motivation and Context

In an effort to make the Python port more idiomatic, let's remove the
`Verify` class and the `./diagnostics` sub-module. There are many more
follow-on tasks to do here, but this is a good start (and already a
large enough change).

### Description

This PR does the following:

1. Removes the `Verify` class, and re-writes all instances of `Verify.*`
2. Adds a `validation.py` in the `./utils` sub-module to hand some of
the more complex cases from `Verify` (checking that skills/functions/and
function params have valid names)
3. Removes the rest of the `./diagnostics` sub-module (w/ a longer-term
goal of removing all/most of our custom exception classes and, instead,
using appropriate built-in error classes)
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
### Motivation and Context

In an effort to make the Python port more idiomatic, let's remove the
`Verify` class and the `./diagnostics` sub-module. There are many more
follow-on tasks to do here, but this is a good start (and already a
large enough change).

### Description

This PR does the following:

1. Removes the `Verify` class, and re-writes all instances of `Verify.*`
2. Adds a `validation.py` in the `./utils` sub-module to hand some of
the more complex cases from `Verify` (checking that skills/functions/and
function params have valid names)
3. Removes the rest of the `./diagnostics` sub-module (w/ a longer-term
goal of removing all/most of our custom exception classes and, instead,
using appropriate built-in error classes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants