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

fix several newline problems #15028

Merged
merged 2 commits into from
Jul 21, 2020
Merged

fix several newline problems #15028

merged 2 commits into from
Jul 21, 2020

Conversation

narimiran
Copy link
Member

No description provided.

@ghost
Copy link

ghost commented Jul 21, 2020

Is there any reason checks are with doAssert and not assert?

@@ -146,7 +146,11 @@ proc send*(ftp: AsyncFtpClient, m: string): Future[TaintedString] {.async.} =
## Send a message to the server, and wait for a primary reply.
## ``\c\L`` is added for you.
##
## You need to make sure that the message ``m`` doesn't contain any newline
## characters. Failing to do so will raise ``AssertionDefect``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In non-release mode, right? Aren't assertions turned off for release mode?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Varriount doAssert assertions are always enabled

@@ -245,7 +245,8 @@ proc contentLength*(response: Response | AsyncResponse): int =
##
## A ``ValueError`` exception will be raised if the value is not an integer.
var contentLengthHeader = response.headers.getOrDefault("Content-Length")
return contentLengthHeader.parseInt()
result = contentLengthHeader.parseInt()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use parseSaturatedNatural because that cannot raise an overflow exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that changes the documented behaviour:

A ValueError exception will be raised if the value is not an integer.

import parseutils, strutils

var a = "123.45"

var b: int
discard parseSaturatedNatural(a, b)
echo b # ==> 123

var c = parseInt(a)
echo c # ==> ValueError

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...

@Araq Araq merged commit 5fafa2f into devel Jul 21, 2020
@Araq Araq deleted the miran-newlines branch July 21, 2020 20:49
narimiran added a commit that referenced this pull request Jul 22, 2020
* prevent newlines where they shouldn't be
* 'contentLength' shouldn't be negative

(cherry picked from commit 5fafa2f)
narimiran added a commit that referenced this pull request Jul 22, 2020
* prevent newlines where they shouldn't be
* 'contentLength' shouldn't be negative

(cherry picked from commit 5fafa2f)
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* prevent newlines where they shouldn't be
* 'contentLength' shouldn't be negative
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.

3 participants