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

do not wrap lines in the upload url fetch #4639

Merged
merged 5 commits into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@guerler
Copy link
Contributor

commented Sep 18, 2017

No description provided.

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

I am open to treating this as a bugfix and getting it to 17.09.

@guerler guerler added kind/bug and removed kind/enhancement labels Sep 18, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

+1 to lump it into 17.09. Is there any reason to do the replace/split('\n') shuffle, and not just split() without args, letting python handle the whitespace logic? (could be a good reason, but at first glance it seems like it'd be simpler and more robust to just do url_paste = url_paste.split())

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

Is that what you meant?

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

for split()

If the optional second argument sep is absent or None, the words are separated by arbitrary strings of whitespace characters (space, tab, newline, return, formfeed).

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

@guerler I meant dropping both replaces and just having the split(). By default, it should cover all whitespace including space, \n, and \r.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

Got it. Thanks.

@guerler guerler added status/review and removed status/WIP labels Sep 18, 2017

@blankenberg

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

But what about a URL that has spaces in that haven't been properly converted to %20?

I know URLs are not technically supposed to have spaces in them, but most modern things are smart enough to handle them: chrome, firefox, wget, curl, etc. In this case I think it could be a matter of usability rather than technically correct.

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

@blankenberg Usability is what we are trying to improve here I think, what do you suggest to do with spaces then?

@blankenberg

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

The help text states You can tell Galaxy to download data from web by entering URL in this box (one per line). You can also directly paste the contents of a file.

I would suggest turning off line-wrapping in the paste/fetch data box, instead have scrollbars -- then uploaded data will match what is shown on the screen to the user.

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

@blankenberg I like that.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

Me too. Thats a good idea. I restored the previous statements and tested the line wrap changes for FF and Chrome.

@galaxyproject galaxyproject deleted a comment from guerler Sep 19, 2017

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

@galaxybot test this

@martenson martenson modified the milestones: 18.01, 17.09 Sep 19, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

@guerler wanna rename this so it reflects its target milestone?

@martenson martenson changed the title [18.01] Replace spaces with newlines in multi url upload requests Replace spaces with newlines in multi url upload requests Sep 19, 2017

@martenson martenson changed the title Replace spaces with newlines in multi url upload requests do not wrap lines in the upload url fetch Sep 19, 2017

@martenson martenson merged commit f154058 into galaxyproject:dev Sep 19, 2017

5 of 6 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@martenson

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

@guerler I took the liberty to do that

@dannon dannon referenced this pull request Oct 30, 2017

Closed

Uploadbox tweaks #4896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.