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

Add readData proc that accepts a string. #9613

Merged
merged 1 commit into from
Nov 7, 2018
Merged

Add readData proc that accepts a string. #9613

merged 1 commit into from
Nov 7, 2018

Conversation

moigagoo
Copy link
Contributor

@moigagoo moigagoo commented Nov 3, 2018

There are two variants of decodeData: one that takes a set of methods and one that takes a string. There is however only one variant of readDate, the one that take a set of methods and calls the corresponding decodeData variant.

I'm currently making a Slack app and need a way to extract a value from a query string. Having a readData proc I could feed a string to and get a table from would be really nice. So I'm adding it with this PR.

@moigagoo
Copy link
Contributor Author

moigagoo commented Nov 3, 2018

One important question. I haven't found any existing tests for cgi module. I remember from the previous PRs that adding new test modules is strongly discouraged in favor of adding tests to the existing ones.

Should I create a test module for cgi? Should I do that in a separate PR?

@timotheecour
Copy link
Member

Should I create a test module for cgi? Should I do that in a separate PR?

tests should almost always be in same PR as functionality change

adding new test modules is strongly discouraged in favor of adding tests to the existing ones.

this may change thanks to #9581 ; just make sure the test doesn't use echo but just doAssert (see that issue) /cc @Araq

@krux02
Copy link
Contributor

krux02 commented Nov 7, 2018

Please add a test. Otherwise I approve this PR.

@moigagoo
Copy link
Contributor Author

moigagoo commented Nov 7, 2018

I'm having trouble writing and running the test. Even when my new test passes, koch tests ends with a failure:

PASS: tcgi.nim C                                                   (2.18800211 secs)
C:\Users\moigagoo\Projects\Nim\testament\tester.exe html
tester.nim(524)          tester
tester.nim(508)          main
htmlgen.nim(130)         generateHtml
htmlgen.nim(73)          allTestResults
json.nim(790)            parseFile
json.nim(774)            parseJson
json.nim(761)            parseJson
parsejson.nim(535)       eat
parsejson.nim(531)       raiseParseErr
Error: unhandled exception: testresults\stdlib.json(6, 205) Error: ] expected [JsonParsingError]
FAILURE

I've put tcgi.nim is tests/foo and run the test with koch tests cat foo (I'll move tcgi.nim to tests/stdlib when it passes).

@moigagoo
Copy link
Contributor Author

moigagoo commented Nov 7, 2018

Also, it passes even with wrong assertions (see the last one):

import cgi, strtabs


const queryString = "foo=bar&фу=бар&checked=✓&list=1,2,3&with_space=text%20with%20space"

let parsedQuery = readData(queryString)

doAssert parsedQuery["foo"] == "bar"
doAssert parsedQuery["фу"] == "бар"
doAssert parsedQuery["checked"] == "✓"
doAssert parsedQuery["list"] == "1,2,3"
doAssert parsedQuery["with_space"] == "text with space"
doAssert parsedQuery["with_space"] == "this is wrong"

@moigagoo
Copy link
Contributor Author

moigagoo commented Nov 7, 2018

OK I've found a way to run a single test file, but the test runner is clearly acting weird.

Test:

doAssert 1 == 2

Run:

.\testament\tester.exe r .\tests\stdlib\tcgi.nim
PASS: tcgi.nim C                                                   (1.72449923 secs)

@Araq
Copy link
Member

Araq commented Nov 7, 2018

Give it a spec section.

@Araq Araq merged commit d5e113c into nim-lang:devel Nov 7, 2018
@moigagoo
Copy link
Contributor Author

moigagoo commented Nov 7, 2018

First, thanks for accepting the PR. Second, I've added the test for the new function: #9645

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.

None yet

4 participants