Chicken-and-egg problem between repos of Deform and deformdemo #472
Comments
|
|
Assuming the repo's are split because of a separation of concerns, I am thinking what are the actual concerns (correct me if I'm wrong):
So the issue is with the source code management, both libraries depend on each other, So how about merging the source code to one (deform would be the logical choice) but keep the separate repo's around for issue tracking and documentation. |
|
First of all, thanks for kicking me in the butt to rethink what we're doing and why. The demo was part of Deform at one point long ago, but it was split out for reasons that are lost to the sands of time, and before I started using Python. I've got my feelers out to hopefully get some insight. These days I'm not concerned about keeping the demo separate, except to archive it, if it were to be brought back into Deform. I would lock down the deformdemo repo, and move all its issues and PRs to Deform (this is easy in GitHub now). Personally I think it is unnecessary overhead that just gets in the way of being a contributor. I've never liked this separation and it does not make sense to me. Let's assume we bring the demo back into Deform. Here are my thoughts.
|
|
I think merging would be a great improvement. If you reformat your thoughts as a checklist we can get started ;) |
|
Sorry guys, I will not contribute in merging Deformdemo to Deform since I think we should separate them. The only reason original designer of Deform removed Deformdemo, was to prevent bloating Deform, but included functional test from Deformdemo to make sure any changes to Deform will pass functional tests. The future of Deform is being simple, and yet independent to any of the javascript, even bootstrap, once we include npm/yarn package manager, and move all javascripts out of Deform, at some point we might move to flexbox instead of bootstrap. The easiest way to over come the inter dependency of Deform and Deformdemo is to remove functional tests from Deform tox.ini, moving all extra widgets to Deformdemo, but knowing that any changes in Deform, shall pass the unit test and as well as functional test in Deformdemo. |
|
I agree that Deform's repo would be bloated. However Deform the released package would not be bloated by merging. @sydoluciani is there harm from bloating the repo that I overlooked? @tdamsma I omitted many details of what a merge would require. It is more complex than my abbreviated overview. I am in no rush to merge the repos. There are many other issues in Deform that have a higher priority over this one, and we have a workaround but it is not documented. The suggestion to remove the
Our contributing.md is hand-wavy in this regard. I will make a PR to improve that section. Here's an attempt to clarify by inserting a new section and a header. I'd appreciate both of your feedback. Functional tests... When to add or edit functional testsIf you add a feature to Deform, or change a feature in Deform that causes any functional test to fail, then you also must submit a pull request that includes sufficient functional tests to the deformdemo repository paired to your pull request in Deform. Before submitting any pull request, all tests must pass using All pull requests automatically run tests. However when there are paired pull requests in both Deform and deformdemo, we have a chicken-and-egg situation. Each repository cannot pull the version of the other referenced in the pull request to run functional tests. Automated testing will fail for both pull requests. We work around this as follows.
Running functional testsTo run functional tests. ... |
|
See PR #474. Review and comments appreciated over there. Out of all this discussion, I might still reorganize the repo for best practices, but leave out the demo, so that I can include only Deform core and not its tests when releasing to PyPI. |
There is no harm if you @stevepiercy and @tdamsma have time to make required changes and maintain it in future, but it definitely increases the maintenance hours in long run, and limits our maneuver ability to implement new strategies in future. Deform facilitates overriding templates or introducing new widgets, but that does not mean for every new widget or new template need to have test. contributors can submit their new widgets or templates to custom_widgets of Deformdemo even without test, and move it to Deformdemo official templates if test is written for them.
@stevepiercy I totally agree with this part. right now , tests are passed against Firefox but failing against Chrome or Opera just because of the way functional tests are written and needs to be fixed. the way Selenium handles click and moving around with arrowup and arrowdown is handled differently in Firefox and Chrome, and some functional tests should be rewritten to use Selection instead of click and arrow. then there are bugs or issues that are not addressed for a while, and on top of that we need to improve or fix issues that is raised after Bootstrap upgrade. with all these I don't think we gain much with merging Deform and Deformdemo, since there are only few pull requests per month.
|
Just want to give my outside perspective on this: I would not be surprised if the
Now I get the sentiment that functional frontend tests should be a this, but in practice, in my experience at least, I have not come across a web project with functional tests that "just work" as expected. Also with Cypress (which makes strings claims about not being flaky, haha) I have experienced great difficulty to run tests consistently. They work (from a docker container) on one machine, and then not in CI (using same docker container) etc. I wasted a few hours getting the simplest widget I wanted to contribute to be properly tested, and then @stevepiercy wasted some more. In that time I could have contributed a lot more valuable stuff to deform. If it were up to me I would:
Ultimately tests are there to help move a project forward, not to hold it back |
I agree in part. There are more reasons that Deform does not have a lot of contributors. More than any other reason, many contributors have moved on to JavaScript frameworks, such as React. Deform has not had a committed maintainer for many years. There has been a lack of follow up on open issues and PRs for about six years. After I finished triaging issues last month, I found that only a couple original contributors gave their time to help resolve the issues they brought up. Before the setup was cumbersome, it was unworkable. The setup was broken for almost two years until @sydoluciani (mostly) and I fixed and documented it, and I became a maintainer just this summer. We're now taking the next step, which is being worked on in either PRs. @sydoluciani is taking that detailed documented process and learned experience, and containerizing it, giving developers an easier option to get started and run. I acknowledge that there are problems. Collectively we collaborate to fix them, according to priorities and available time.
I learned that drag and drop with Selenium does not always work. I also followed the path you took to ultimately choose the Select2Sortable plugin, and gained a greater understanding of how others found different solutions that worked for them, some of which found happiness in jQueryUI's draggable option. I would not call that a waste of time.
We need something that can test manipulation of the DOM, such as a Sequence (of Mappings) with Ordering Enabled. I do not know if Webtest can do that. If it can replace Selenium while maintaining 100% functional test coverage, I would accept pull requests to change. I am open to other options than Selenium, too.
Tests are there to preserve the quality of a project and prevent it from regressing. Perhaps I misunderstand your statement, but are you saying that we should change the Pylons Project official project policy of 100% test coverage? |
Selenium will exist even after the merge, therfore contribution instruction still will be the same.
Agree with your point, and not every contributor wants to be involved with Selenium or any kind of testing, not because it is not a good thing, but because it is cumbersome and need to spend time, but again merging won't help, since merge means we are merging selenium tests from Deformdemo to Deform. you could just submit a pull request to Deformdemo for your custom widgets, place it in custom_template with no test. if contributors can just submit their custom widgets with no test in Deformdemo, we have way more widgets that we have now. what you did with demonstrating your widgets in Deformdemo was great, it is showing how contributors can introduce new resource in resource registery and creating new widgets and we can use it as an example and base for new contributions.
This is another change on top of merging that needs its own implementation time and effort, and even if we have time for both, not advisable to do both at the same time. but now that I am thinking, you have a good point, Deform only responsible to get the data and produce sequence or map of field objects and unit test with minimum web test shall suffice. if you have time, why not write a webtest completely for Deform, and make Deformdemo a separate entity with structure that people can submit their widgets and templates easily with no test if it stays in custom_widget and if they write test, we move it to official template folder of Deformdemo ? please let us know what framework or language you were going to choose for webtest if you had the time ?
|
When I say no test, I mean minimum test or at least working in Deformdemo. as long as other contributors can use these custom widgets to have a faster start in their projects or in contributing back to Deformdemo
|
|
Let me start off by explicitly expressing my gratitude to @stevepiercy and @sydoluciani for reviving deform.
@stevepiercy I admire your optimism! Indeed I also learned a bit in the process.
I was not advocating for lowering test coverage, but to rethink the scope of what needs to be tested (and leave the functional/selenium part out). I think @sydoluciani agrees:
To wrap things up, I will work on adding my custom widgets to deformdemo in the propose structure. When @stevepiercy and @sydoluciani agree on a way forward, I will gladly chime in again and see what I can contribute to Deform. |
I think for new wedgets if they placed in custom_templates that represents un-official Deformdemo widgets, there should be no test requirement to bring in more contributores to load their custom widgets and templates. as long as it is functional in Deformdemo. if other users started using these new widgets or templates, they will create issues indicating there is a need for better support or coverage, then we can work on new feature or test coverage and move them to official directory within Deformdemo. At the end of the day, @stevepiercy is making the last call since he is the product maintainer.
Thank you @tdamsma , your custom widgets can be a base for new contributors. |
|
The single existing custom widget was implemented without much thought toward expansion. The following are my thoughts about how to improve the situation for expansion with more custom widgets. Here's the current structure:
Going forward, I would like to see.
|
So the process for custom resources (jss, css) is now
And all this for what actually? I don't understand the design, it seems to allow for decoupling between resource and widget. But they are by nature quite tightly coupled. Perhaps if there could be a parallel registry that uses BS4 instead of BS3? But then that would also require a different template, so basically you would need to change the entire widget altogether. If at all possible, it would make life a lot easier if the css and js resources could be defined on the Widget class, just like the template. Basically replacing the |
|
Yeah, I was a little too hand-wavy about static resources with this one, so I just now edited it.
But I think you can still work with it. In the widget docs, does the |
|
So that allows overriding the default registry. So when we add multiple custom widgets to deformdemo they will have to share the same resource registry. Supporting js_resources and css_resources could be a non breaking deform change right? Why is the resource registry implemented the way it is, am I overlooking something or is it to put it bluntly a useless, overengineerd abstraction? I can't think of a usecase where it would come in handy found this package that uses it: https://github.com/robinharms/deform_autoneed. Don't really know what it does and seems pretty abandoned |
|
I don't quite follow you. Is your frustration with the resource registry itself or how the resources are rendered in the page? I would have to dig into history and git comments to understand why the resource registry was designed this way. I never gave it much thought, assuming that the original authors had good reasons to do so. I agree that the rendering is sub-optimal. I think https://github.com/fanstatic/js.deform solves the rendering issue, and Fanstatic resolves a few other issues. |
|
Here are some commits that mention resource. There was even an attempt back in 2013 to autogenerate assets with bower (RIP). |
|
My point is that there is a layer of abstraction (the resource_registry) that forces you to split things that logically belong together (paths to resources and widgets that require them), and that makes combining widgets from different sources difficult (because you you then have multiple registries, which isn't supported). So this abstraction has a cost, but it has (as far as I can see) no benefit. |
|
I don't have an answer for you. Sorry. You'll have to ask the original authors or co-conspirators at that time. I can insert a key/value into the resource_registry, so I don't see the difficulty you describe. I'm trying to understand your beef, but not doing well. Maybe I don't understand your definition of "combining widgets". Do you mean having multiple widgets in a form or page, grabbing a jQuery plugin and all of its dependencies to create a widget, or something else? |
|
ideally I would like to have all the code for an example widget in a single folder/module. Then in an application i do |
|
I agree with all the points @tdamsma is making, but these will go away if we move toward webpack/npm/yarn. Deform originally started in 2010, just after node.js first release in 2009 , and at that time, original author wanted to make it easy for end user to load proper css or javascript for each widget with just calling get_widget_resources() on widget, and then load it in template as it explained in form tutoiral, search for reqts in document: Without having resource registery, end user should have guessed the proper javascript or css file name based on widget name, and looking in to static directory and try and error. The same functionality will be provided, using Webpack. Webpack will transpile all javascript and css files in one file, then copies it in static directory, and we include that one file in our widget resource or in our page. Probably we should talk about resource registery when we have decided to move to Webpack or other tools that provide some sort of registry or automation. |
I am not seeing any Bootstrap css or Bootstrap javascript in either requirements, or registery !!!! From what I understood, Bootstrap classes are statically injected in templates, and not loaded through widget resource registery, and that is the reason Bootstrap files are included statically and not through resource registary: But then that would also require a different template, so basically you would need to change the entire widget altogether.
|
|
I agree it is a bit confusing, but if we move all those widgets from widget.py to Deformdemo, other than Widget class, SequenceWidget class and MappingWidget class, Deform itself has no dependency to any of the CSS or Javascripts. We upgraded Bootstrap with no problem if excluding failed tests left from previous works, and any changes we made so far to fix or improve was in tests and functional tests. I only changed 3 lines of code in Deform so far. |
It needs a redesign for sure, but we should consider using Webpack or other frontend tools to provide the resources to Deform. I would leave it until we have the proper frontend tool ready and discussed. |
Unless you want to bundle all js and css for all widgets into a single file I don't see what the build system has to do with this. A better build system could help generate up to date minimal js/css per widget built from source instead of vendored in code via copy/paste. But still every widget would have to somehow define which js/css files need to be loaded to make it work. |
|
@tdamsma I think I understand the problem you describe now. Thanks for explaining. I don't know of a good way forward, but I'm open to suggestions for a plan forward and a proof of concept. @sydoluciani indeed, Bootstrap is not registered in Deform, but only included in its package. In my apps, I use Jinja2 with Bootstrap requirements specified as static assets in the Whoever solves the "JavaScript frontend dependency integration with Python schema declaration backend" problem will be wealthy beyond their wildest dreams. Perhaps Fanstatic or deform_autoneed has already solved this issue? I could reach out to both maintainers and ask them some questions. If you have some questions that you'd like to ask, I can collect them and be liaison. I can also preface my questions with the current state of Deform and options of where we would like it to move toward.
|
Fantastic uses intermediary Python library like js.bootstrap to load the Bootstrap JavaScript and CSS, and I think it is redundant, however my question to Fantastic developers is, if they have thought of writing a Python wrapper around Webpack instead to produce front end skelton for Python web frameworks. like being able to download, and produce output by instantiating a Python class or calling a Python function instead of running npm and webpack manually. I was going to say we don't need any kind of translatore that found this package Looked up for Python Webpack Wrapper and found this package and then this package but they both make the solution more complex than it is now. |
|
Discussion opened in fanstatic/js.deform#24 |
@tdamsma @sydoluciani put some work into a structure that we think is a pretty good compromise in Pylons/deformdemo#92 which is now on the deformdemo master branch (I will backport it to the 2.0-branch later). |
|
I liked what @tdamsma suggested for having separate module for each Deformdemo form, like the directory structure @stevepiercy suggested, instead of having all Deformdemo forms in @tdamsma can either include new Deformdemo forms and widgets with replacing Text Input Widget, or re-architect within un-official Deformdemo diretory structure and include new Deformdemo forms. not a bad idea to have an alternative example for Deformdemo for future reference. |
|
This issue has wandered off course, so I'll attempt to round it all up. Deform and DeformDemo 2.0.15 were both released last week, and there were a ton of improvements. Effectively we cannot solve the original issue while there are two separate repos. That's OK. To mitigate the issue and other points brought up during the discussion, note the following.
@tdamsma it would be awesome if you were to contribute your Select2Sortable widget to the Unofficial Deform Demo. I think that covers it all. |
Not by itself, containers just make running the tests more easily reproducible across different systems. The core problem I think is that deform and deformdemo are by there nature tightly coupled, so their releases, PR's, features etc should be completely in sync. The best way I know how to do that is a monorepo; i.e. merging deformdemo into deform. I would understand if that is not an option, but I think that is fundamentally the easiest solution.
But to keep the current split, a few options crossed my mind:
create a "next" branch on both repositories that is used in the tests. The next branch is either equal to master, or has some extra's bolted on to progress to the next step. In the deform repo a PR to master is tested using the next branch of deformdemo. A PR for deformdemo@master is tested using deform@next. So a new feature (that influences both repo's) requires PR's to be made to both next branches. These PR's are merged without testing, and then new PR's are made for merging next into master. This all feels quite cumbersome, especially when iterations are needed to get stuff ready. Also this doesn't allow multiple PR's for different features to co-exist. Effectively what I would want is that when I make a PR into Pylons/deform@master from tdamsma/deform@, the tests are run using tdamsma/deformdemo@<some-new-feature and not Pylons/deformdemo@master. Probably technically possible to set up, but it sounds way to convoluted
use git submodules and tests against whatever the current commit of the submodule is. I am not so experienced with this so not really sure if this would actually work
require a certain order for changes: first commit/merge a PR in deformdemo, and only the make a PR for deform. Use some kind of decorator to skip tests for unavailable features depending on the deform version, something like
pytest.mark.skipif(deform.__version__ <= "3.11")To be honest I don't like any of these options much. Perhaps this should be a separate discussion for the containerization?
Originally posted by @tdamsma in #471 (comment)
The text was updated successfully, but these errors were encountered: