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

required children prop #2218

Merged
merged 19 commits into from
Sep 12, 2022
Merged

required children prop #2218

merged 19 commits into from
Sep 12, 2022

Conversation

siner308
Copy link
Contributor

@siner308 siner308 commented Sep 5, 2022

children prop cannot be REQUIRED prop now.
This PR makes it possible to set required to children prop.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Reformat old codes that broken at flake8 test
    • Make children prop checkable in __init__ function of python component
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@siner308 siner308 changed the title Required children prop make children prop required Sep 5, 2022
@siner308 siner308 changed the title make children prop required required children prop Sep 5, 2022
siner308 and others added 2 commits September 6, 2022 06:39
Co-authored-by: Minsu Kim <alstn2468_@naver.com>
Co-authored-by: Minsu Kim <alstn2468_@naver.com>
Copy link
Contributor

@alstn2468 alstn2468 left a comment

Choose a reason for hiding this comment

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

image

LGTM

@alstn2468
Copy link
Contributor

@alexcjohnson I want to add test case to test_base_component.py, but is there a specific rule for test case name such as debcXXX?

@alexcjohnson
Copy link
Collaborator

@alexcjohnson I want to add test case to test_base_component.py, but is there a specific rule for test case name such as debcXXX?

Thanks @alstn2468! Yes, across dash the test case names are generally two characters for the folder (development -> "de") then two characters for the filename (test_base_component -> "bc") then a 3-digit serial number. So if you call your test test_debc030_whatever you can run it solo with pytest -k debc030

That said, given that this involves the generator, it might be better to make a new component with required children in @plotly/dash-test-components, and test it in maybe tests/integration/test_generation?

@siner308
Copy link
Contributor Author

siner308 commented Sep 8, 2022

@alexcjohnson I want to add test case to test_base_component.py, but is there a specific rule for test case name such as debcXXX?

Thanks @alstn2468! Yes, across dash the test case names are generally two characters for the folder (development -> "de") then two characters for the filename (test_base_component -> "bc") then a 3-digit serial number. So if you call your test test_debc030_whatever you can run it solo with pytest -k debc030

That said, given that this involves the generator, it might be better to make a new component with required children in @plotly/dash-test-components, and test it in maybe tests/integration/test_generation?

I added debc030 for testing invalid children argument in 4800dcb

@alexcjohnson
Copy link
Collaborator

@siner308 @alstn2468 this is looking great. I'm not sure though why CircleCI tests don't run... normally we have no problem with PRs coming from forks, any chance there's a setting in your fork that could be blocking this? I know there's the Allow Edits from Maintainers option but even if you haven't selected that I wouldn't have expected that to matter here. @T4rk1n any ideas?

We'll want a changelog entry, which begs the question whether this is a feature addition or a bugfix. I could see calling this a bugfix, since we never said you COULDN'T set children as required so it's implied that it should work. And this has the advantage that we'd be able to release it sooner, as we'll be ready to make a patch release in the next few days but we'll want to wait a little longer before making a new minor.

@alstn2468
Copy link
Contributor

alstn2468 commented Sep 8, 2022

@alexcjohnson I think you can check the "Build forked pull requests" option.
https://circleci.com/blog/triggering-trusted-ci-jobs-on-untrusted-forks/
https://circleci.com/docs/oss#build-pull-requests-from-forked-repositories

@alexcjohnson
Copy link
Collaborator

As I said, generally PRs from forks do work - see eg #2182 where a new build worked in between failed builds from this PR - so there's something else going on here.

What I see in the CircleCI project page is a bunch of messages like:

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.Please sign out of CircleCI and log back in with your VCS before triggering a new pipeline.

Which doesn't make a lot of sense given that other builds are still working fine and you didn't touch config.yml. I'll reach out to CircleCI about this, but worst case we just push this branch to the main repo and make a new PR.

@siner308
Copy link
Contributor Author

siner308 commented Sep 8, 2022

There are more ci waiting PRs in this repository. (e.g. #1564)
Is there any commonality?

Or I can make a new PR from same source for check CI.

@alexcjohnson
Copy link
Collaborator

Seems to be working now, after I followed CircleCI's instructions. Hopefully it'll work for your commits too, not just the one I added by merging dev!

So now we can see a couple of tests failed: here if you can see it, both failures are in tests/unit/development/test_generate_class_file.py - probably just the tests need updating to reflect the adjusted generator, rather than a problem with the source code. The test-36 failure is spurious.

Aside from that I think we just need a changelog entry then we'll be ready to go :)

@siner308
Copy link
Contributor Author

siner308 commented Sep 9, 2022

I just fix metadata_test.py and add changelog.
As a result, two failed test cases reported from CircleCI is passed in my local (test_class_string, test_class_file)

But CI is still not working from my commit 😥

@siner308 siner308 changed the base branch from dev to antrg/ci-warnings September 9, 2022 18:37
@siner308 siner308 changed the base branch from antrg/ci-warnings to dev September 9, 2022 18:37
@siner308
Copy link
Contributor Author

siner308 commented Sep 9, 2022

I tried to setup circleci from my forked repo and pipeline works here.
But is no orb percy/agent in orbs list and percy/percy-agent repo in github is already deprecated 16 months ago.
I think this is why I failed to run ci in my forked project. Anyway, CI is triggered without problem!

I'm not sure this is a reason that cannot run in this PR.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Looks great. Thanks again @siner308 @alstn2468! I still don't know what's going on with CircleCI and your commits, and their support folks haven't come up with any useful suggestions. But it worked when I added my own commit and all tests pass, so I think we can move on.

@alexcjohnson alexcjohnson merged commit 6481306 into plotly:dev Sep 12, 2022
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.

3 participants