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

Fix missing check in validator #1712

Merged
merged 7 commits into from
May 24, 2024

Conversation

lucas-flexcompute
Copy link
Collaborator

No description provided.

Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
@tylerflex
Copy link
Collaborator

I wonder if the Union fields need
discriminator=TYPE_TAG_STR

?

@lucas-flexcompute
Copy link
Collaborator Author

I'm not sure. It seems to happen only on Windows.

@lucas-flexcompute
Copy link
Collaborator Author

I wonder if the Union fields need discriminator=TYPE_TAG_STR

I think the unions cannot have a discriminator, because a list won't have a type attribute. That's the reason why I created the annotated medium type, so it could be used within the list.

Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
@tylerflex tylerflex added the 2.7 will go into version 2.7.* label May 23, 2024
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
@lucas-flexcompute
Copy link
Collaborator Author

The only test that is failing is the submodule. I've already updated it locally, but it doesn't seem to make any difference. @daquinteroflex @tylerflex any idea how to fix?

@tylerflex
Copy link
Collaborator

I think possibly your first Multilayer waveguide PR was failing submodules too, so pre/2.7 might be already out of date. Let me push a submodule update to pre/2.7 and see if it fixes things.

@tylerflex
Copy link
Collaborator

shoot, that didnt fix it maybe @daquinteroflex can take a look when he has time

@tylerflex
Copy link
Collaborator

before merging (once the submodule test is fixed) just a reminder to squash all of the commits into one if you dont mind.

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Approve pending test passing and squashed commits.

Also curious what the issue was?

@lucas-flexcompute
Copy link
Collaborator Author

Approve pending test passing and squashed commits.

Also curious what the issue was?

The problem was with isinstance(..., MediumType). For some reason that doesn't work on Windows. So I replaced all checks with isinstance(..., "some concrete type"). I've also changed all instance validators to being root validators because I think that was also triggering the same issue internally.

@daquinteroflex
Copy link
Collaborator

Hello! Sorry about that, that's weird. I think I've fixed it now and for reference these are the commands I run to update it:

 1004  git checkout lucas/waveguide_plugin/test_fix
 1005  cd docs/
 1006  cd notebooks/
 1007  git pull origin pre/2.7 
 1008  cd ..
 1009  git add .
 1010  git commit -am ":wrench: Update submodule?"
 1011  git log
 1012  git push origin lucas/waveguide_plugin/test_fix 
 1013  history

@lucas-flexcompute lucas-flexcompute marked this pull request as ready for review May 24, 2024 09:32
@lucas-flexcompute lucas-flexcompute merged commit 38b9f93 into pre/2.7 May 24, 2024
16 checks passed
@lucas-flexcompute lucas-flexcompute deleted the lucas/waveguide_plugin/test_fix branch May 24, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 will go into version 2.7.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants