maximilianh / cellBrowser

main repo: https://github.com/ucscGenomeBrowser/cellBrowser/ - Python pipeline and Javascript scatter plot library for single-cell datasets, http://cellbrowser.rtfd.org
https://github.com/ucscGenomeBrowser/cellBrowser/
GNU General Public License v3.0
102 stars 40 forks source link

getDatasetNameFromUrl() in cellBrowser.js converts most datasetName to lowercase #219

Closed bugacov closed 2 years ago

bugacov commented 3 years ago

The logic in this function will converts pretty much all datasets to lowercase so if one creates a dataset w/ uppercase name it throws an error saying that it cannot find the dataset when passing a URL with a dataset name (last argument) in uppercase

maximilianh commented 3 years ago

Yes, and this is intentional, because at UCSC we only allow lowercase dataset names. I agree that I should document this and possibly allow a different handling.

The reason for all-lowercase is that we serve the datasets using an Apache rewrite rule, which allos URLs like cortex-dev.cells.ucsc.edu and DNS does not support case, it may come from a time when there were still 7bit codepages around.

What do you suggest that I do for non-UCSC users?

1) throw an error when an upper case is used in a dataset name 2) deactivate the lower casing on non-UCSC systems

Is that upper case / lower case question very important for your use case? Could you live with all-lowercase datasetnames, knowing that you could make the URLs easier to read (I can document how we configured our Apache, I think it's a one-line .htaccess file)

Thanks for opening this ticket. It's definitely something I should have expected happening to other users...

On Wed, May 26, 2021 at 3:33 AM bugacov @.***> wrote:

The logic in this function will converts pretty much all datasets to lowercase so if one creates a dataset w/ uppercase name it throws an error saying that it cannot find the dataset when passing a URL with a dataset name (last argument) in uppercase

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/maximilianh/cellBrowser/issues/219, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACL4TKIHFULQBQJW5CZUO3TPRFX5ANCNFSM45QSE4CQ .

bugacov commented 3 years ago

Hi Maximilian, Thanks for your quick reply!! Your option 2) :

2) deactivate the lower casing on non-UCSC systems

Is what we suggest implementing. The part of the URL after the ?ds= is case sensitive and works well for when I tested modifying your code in my installation.

We’d really appreciate if you can implement that change. Thanks again for your help!! Best, Alejandro.

From: Maximilian Haeussler @.> Reply-To: maximilianh/cellBrowser @.> Date: Wednesday, May 26, 2021 at 1:57 AM To: maximilianh/cellBrowser @.> Cc: Alejandro Bugacov @.>, Author @.***> Subject: Re: [maximilianh/cellBrowser] getDatasetNameFromUrl() in cellBrowser.js converts most datasetName to lowercase (#219)

Yes, and this is intentional, because at UCSC we only allow lowercase dataset names. I agree that I should document this and possibly allow a different handling.

The reason for all-lowercase is that we serve the datasets using an Apache rewrite rule, which allos URLs like cortex-dev.cells.ucsc.edu and DNS does not support case, it may come from a time when there were still 7bit codepages around.

What do you suggest that I do for non-UCSC users?

1) throw an error when an upper case is used in a dataset name 2) deactivate the lower casing on non-UCSC systems

Is that upper case / lower case question very important for your use case? Could you live with all-lowercase datasetnames, knowing that you could make the URLs easier to read (I can document how we configured our Apache, I think it's a one-line .htaccess file)

Thanks for opening this ticket. It's definitely something I should have expected happening to other users...

On Wed, May 26, 2021 at 3:33 AM bugacov @.***> wrote:

The logic in this function will converts pretty much all datasets to lowercase so if one creates a dataset w/ uppercase name it throws an error saying that it cannot find the dataset when passing a URL with a dataset name (last argument) in uppercase

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/maximilianh/cellBrowser/issues/219, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACL4TKIHFULQBQJW5CZUO3TPRFX5ANCNFSM45QSE4CQ .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/maximilianh/cellBrowser/issues/219#issuecomment-848596265, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACQMXWI3DMPMY4KANXXGOG3TPSZWHANCNFSM45QSE4CQ.

maximilianh commented 3 years ago

Hi, I've deactivated lower casing, in cellbrowser.js line 7083 now for non-UCSC servers. I cannot test this myself, because all my webservers run at UCSC. If you want, you can checkout the "develop" branch and then run src/cbUpgrade --code -o and try if this solves your problem.

thanks Maxy

On Wed, May 26, 2021 at 7:01 PM bugacov @.***> wrote:

Hi Maximilian, Thanks for your quick reply!! Your option 2) :

2) deactivate the lower casing on non-UCSC systems

Is what we suggest implementing. The part of the URL after the ?ds= is case sensitive and works well for when I tested modifying your code in my installation.

We’d really appreciate if you can implement that change. Thanks again for your help!! Best, Alejandro.

From: Maximilian Haeussler @.> Reply-To: maximilianh/cellBrowser @.> Date: Wednesday, May 26, 2021 at 1:57 AM To: maximilianh/cellBrowser @.> Cc: Alejandro Bugacov @.>, Author @.***> Subject: Re: [maximilianh/cellBrowser] getDatasetNameFromUrl() in cellBrowser.js converts most datasetName to lowercase (#219)

Yes, and this is intentional, because at UCSC we only allow lowercase dataset names. I agree that I should document this and possibly allow a different handling.

The reason for all-lowercase is that we serve the datasets using an Apache rewrite rule, which allos URLs like cortex-dev.cells.ucsc.edu and DNS does not support case, it may come from a time when there were still 7bit codepages around.

What do you suggest that I do for non-UCSC users?

1) throw an error when an upper case is used in a dataset name 2) deactivate the lower casing on non-UCSC systems

Is that upper case / lower case question very important for your use case? Could you live with all-lowercase datasetnames, knowing that you could make the URLs easier to read (I can document how we configured our Apache, I think it's a one-line .htaccess file)

Thanks for opening this ticket. It's definitely something I should have expected happening to other users...

On Wed, May 26, 2021 at 3:33 AM bugacov @.***> wrote:

The logic in this function will converts pretty much all datasets to lowercase so if one creates a dataset w/ uppercase name it throws an error saying that it cannot find the dataset when passing a URL with a dataset name (last argument) in uppercase

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/maximilianh/cellBrowser/issues/219, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AACL4TKIHFULQBQJW5CZUO3TPRFX5ANCNFSM45QSE4CQ>

.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub< https://github.com/maximilianh/cellBrowser/issues/219#issuecomment-848596265>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/ACQMXWI3DMPMY4KANXXGOG3TPSZWHANCNFSM45QSE4CQ>.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/maximilianh/cellBrowser/issues/219#issuecomment-848948211, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACL4TMLBYA5ZTYI66YZSG3TPUSM3ANCNFSM45QSE4CQ .

ljpearlman commented 2 years ago

Hi,

I think !pageAtUcsc() should just be pageAtUcsc() here -- as written, this will lower-case non-UCSC URLs (and not lower-case UCSC URLs).

        if (datasetName && datasetName!=="adultPancreas" && !pageAtUcsc())
                    datasetName = datasetName.toLowerCase();

Thanks,

Laura

maximilianh commented 2 years ago

Yes, this makes sense. I've changed this now. you can get the change from Github. It's not too unlikely that we'll have a release next week, too. Thanks!

On Thu, Aug 19, 2021 at 7:25 AM ljpearlman @.***> wrote:

Hi,

I think !pageAtUcsc() should just be pageAtUcsc() here -- as written, this will lower-case non-UCSC URLs (and not lower-case UCSC URLs).

    if (datasetName && datasetName!=="adultPancreas" && !pageAtUcsc())
                datasetName = datasetName.toLowerCase();

Thanks,

  • Laura

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/maximilianh/cellBrowser/issues/219#issuecomment-901620723, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACL4TM3N5XKS6I4UG7DIRDT5SIUHANCNFSM45QSE4CQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

ljpearlman commented 2 years ago

That worked, thanks!