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

Remove html producing grid helpers #5212

Merged
merged 64 commits into from Dec 15, 2017

Conversation

Projects
None yet
4 participants
@guerler
Contributor

guerler commented Dec 14, 2017

This PR separates the grid helper of Galaxy from the one used in the Reports and Toolshed apps. The Galaxy grid helper does not use any makos but instead builds dictionaries. Transforming the Reports app is most likely trivial but transforming the Toolshed app could prove to be more difficult due to customizations implemented in mako. This PR suggests to achieve separation by copying the legacy mako, js and python grid building modules to the reports app and then to degrade the existing grid helper in Galaxy. This separation will allow us to rewrite the grid client of Galaxy and modify the grid helper without breaking existing legacy grids in the other apps. Certain now unused grid attributes like dict_format, preserve_state, use_async, template, async_template and pass_through_operations have already been removed.

guerler added some commits Dec 9, 2017

@dannon

This comment has been minimized.

Member

dannon commented Dec 14, 2017

As mentioned out of band, the complete duplication of ~2000 lines of the codebase makes me uneasy here. I just want to be sure this is how we want to handle toolshed and reports grids (just leaving them behind, forever?), before we inherit that maintenance burden.

@guerler

This comment has been minimized.

Contributor

guerler commented Dec 14, 2017

In my opinion the Toolshed app should be rewritten from scratch and integrated into Galaxy's admin panel as api driven js-client until then I would argue that its better to move the code it requires out of Galaxy into the Toolshed app. This will allow us to work on replacing the Galaxy grids with a vue component right after this PR. The alternative would be to rewrite the Toolshed first.

@dannon

This comment has been minimized.

Member

dannon commented Dec 14, 2017

I'm with you on the toolshed being rewritten from scratch, being much more tightly integrated with the Galaxy application itself; I've been arguing that perspective since at least 2014. That hasn't moved, at all, in the past 3-4 years though, and I am just worried about being saddled with duplicated code for the next N years, having to fix bugs in both places, etc., instead of narrowing the code paths.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 14, 2017

I'm vaguely uneasy the way @dannon is as well. I think the comment:

In my opinion the Toolshed app should be rewritten from scratch and integrated into Galaxy's admin panel as api driven js-client

doesn't reflect that we still need a browser based landing page for the TS I think. At least we have said that is the case in the past - people who Google "Galaxy BWA" should see some information about what is in the tool shed for BWA I think. I also don't think it is realistic that we will have the resources to that rewrite the tool shed anytime soon. It isn't usually brought up as a project priority anymore.

I would say it is acceptable to duplicate the code if we plan to clearly branch and bring them in different directions but I'd feel more comfortable with this if the grids package was duplicated into something called "legacy" on the path instead of "reports" - since these grids are shared between reports and tool shed.

@guerler

This comment has been minimized.

Contributor

guerler commented Dec 14, 2017

I think it still can have its own browser landing page, we could consider integrating it as separate js-app similar to how the admin panel works. Regarding the reports app, I think its not difficult to transform it, I'd be willing to do that and then move the legacy-grid code further into Toolshed-only sections or we drop the reports app entirely and integrate it into the admin app if thats possible.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 14, 2017

Regarding the reports page, I think its not difficult to transform it, I'd be willing to do that and then move the legacy-grid code further into Toolshed-only sections.

I would be even more comfortable with that I think. If six months from now Galaxy only uses the API to talk to the TS and the TS grids are only used to render the page for users coming from outside of Galaxy - I don't think we would ever have to touch those grids again and I think it would be very clear what parts of the code base used those older grids and what parts used what modern thing you have planned. I think that would be a good direction to take this.

@dannon

This comment has been minimized.

Member

dannon commented Dec 14, 2017

Ok. Opening a PR that will resolve the conflicts and fix a few things, and then we can just merge this and move on.

@dannon dannon merged commit 9aa596d into galaxyproject:dev Dec 15, 2017

6 checks passed

api test Build finished. 334 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 165 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 60 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 117 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment