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

follow #15357 and move decodeQuery #15860

Merged
merged 15 commits into from
Dec 27, 2020
Merged

follow #15357 and move decodeQuery #15860

merged 15 commits into from
Dec 27, 2020

Conversation

ringabout
Copy link
Member

decodeData is very common to use and should work in JS backend(cgi doesn't work in JS backend).

So I move decodeData to std/uri and rename it to decodeQuery. Move CgiError etc to std/uri to keep backwards compatibility.

Or we should continue #15357 and change proc to iterator.

@timotheecour
Copy link
Member

Change it to iterator and make cgi call this

lib/pure/uri.nim Outdated Show resolved Hide resolved
lib/pure/cgi.nim Outdated Show resolved Hide resolved
lib/pure/uri.nim Outdated Show resolved Hide resolved
lib/pure/uri.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Nov 9, 2020

Sorry but this seems to be a bad idea.

"Added decodeQuery to std/uri and deprecated decodeData in std/cgi."

Omg why? It's std/cgi, don't deprecate things in there. Stop messing around with the details -- some people here try to write code against the std libraries...

@ringabout
Copy link
Member Author

So we tend not to deprecate duplicated things in stdlib and only deprecate wrong/outdated things in stdlib?

@Araq
Copy link
Member

Araq commented Nov 9, 2020

Your code introduced the duplication as far as I can tell. We should avoid both the duplication and the deprecation process.

@timotheecour
Copy link
Member

timotheecour commented Nov 9, 2020

Your code introduced the duplication as far as I can tell. We should avoid both the duplication and the deprecation process.

as of right now, only the API declaration is duplicated between cgi and uri, not the implementation, that's an acceptable tradeoff vs the benefits it brings:

decodeData is very common to use and should work in JS backend(cgi doesn't work in JS backend).

plus discoverability, decodeQuery really does belong in std/uri. And this isn't the 1st PR attempting to address this.

So the current version of this PR is fine IMO, not deprecating cgi.decodeData (but uri.decodeQuery could be mentioned in docs for cgi.decodeData) and introducing uri.decodeQuery

@ringabout
Copy link
Member Author

ringabout commented Nov 10, 2020

ECgi* = object of EIO  ## the exception that is raised, if a CGI error occurs

ECgi has changed to CgiError, but some docs didn't change. I will clean them after this PR.

Does decodeQuery need since? I try to add since(1, 5, 1) but failed in CI. It told me that

/home/runner/work/Nim/Nim/lib/pure/cgi.nim(88, 29) Error: undeclared identifier: 'decodeQuery'
FAILURE

@timotheecour
Copy link
Member

timotheecour commented Nov 10, 2020

Does decodeQuery need since? I try to add since(1, 5, 1) but failed in CI. It told me that

probably bootstrap but can't reproduce locally (bin/nim_csources c koch works for me); can you provide a full link to CI logs? I can see several ways to handle this, including when Nim >= (1,5,1) or Nim == (0,20,0) (in pseudocode), depending on the CI log error

@ringabout
Copy link
Member Author

ringabout commented Nov 10, 2020

@timotheecour
Copy link
Member

timotheecour commented Nov 10, 2020

ok, I can reproduce (locally too, via rm bin/nim and booting from csources); this would be easily fixed via {.privateImport.} (see #11865):

# uri.nim:
iterator decodeQuery(data: string): tuple[key, value: TaintedString] = ...
since (1,5,1): export decodeQuery

# cgi.nim
from uri {.privateImport.} import decodeQuery

(or equivalently, via iterator decodeQueryImpl(data: string): tuple[key, value: TaintedString] = ...; template decodeQuery*(data: string) {.since: (1,5,1).}: untyped = decodeQueryImpl(data)
and in cgi.nim: from uri {.privateImport.} import decodeQueryImpl)

but until then the simplest IMO is to just leave out since in this case.
EDIT: I've rebased #11865 so this option can still be on the table if needed (now or later)

@timotheecour
Copy link
Member

needs rebase against devel for conflict

lib/pure/uri.nim Outdated Show resolved Hide resolved
lib/pure/uri.nim Outdated Show resolved Hide resolved
lib/pure/uri.nim Outdated Show resolved Hide resolved
@Araq Araq merged commit 6895040 into nim-lang:devel Dec 27, 2020
ringabout added a commit to ringabout/Nim that referenced this pull request Dec 27, 2020
ringabout added a commit to ringabout/Nim that referenced this pull request Dec 27, 2020
ringabout added a commit to ringabout/Nim that referenced this pull request Dec 27, 2020
Araq pushed a commit that referenced this pull request Dec 27, 2020
* follow #15860  clean cgi module

* follow #15860  clean cgi module
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* follow nim-lang#15357 and move decodeQuery
* solve problem one
* minor
* deprecate decodeData
* add changelog and since
* add testcase for decodeQuery
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* follow nim-lang#15860  clean cgi module

* follow nim-lang#15860  clean cgi module
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* follow nim-lang#15357 and move decodeQuery
* solve problem one
* minor
* deprecate decodeData
* add changelog and since
* add testcase for decodeQuery
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* follow nim-lang#15860  clean cgi module

* follow nim-lang#15860  clean cgi module
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.

4 participants