Skip to content

Conversation

JackStouffer
Copy link
Contributor

I need this for my date string parsing library.


// avoid auto-decoding overhead
foreach (item; s.representation)
if (!isASCII(item))
Copy link
Member

Choose a reason for hiding this comment

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

return s.representation.find!(x => !isASCII(x)).empty

Copy link
Contributor

Choose a reason for hiding this comment

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

or return s.representation.all!isASCII();

Copy link
Member

Choose a reason for hiding this comment

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

Yes, @JackStouffer move to return s.representation.all!isASCII();. This will create less template bloat

@DmitryOlshansky
Copy link
Member

Truth be told it's a hard sell:

  • one liner
  • wrong module, should be std.string
  • trivial

@@ -25,12 +25,12 @@
+/
module std.ascii;

import std.traits; // : functionAttributes, FunctionAttribute, isSafe, isSomeString;
Copy link
Member

Choose a reason for hiding this comment

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

The bug is fixed now

@JackStouffer JackStouffer force-pushed the issue11356 branch 2 times, most recently from 374a403 to fb81279 Compare April 6, 2016 15:42
@JackStouffer
Copy link
Contributor Author

@DmitryOlshansky There were two separate bugs for this and a forum thread, so I think the demand is high enough for this. Fixed other issues.

@9il The bug is only deprecated, imports will still be public if you use global selective imports. From the change log:

To ease updating existing code, the old behavior was retained but deprecated.

@JackStouffer JackStouffer force-pushed the issue11356 branch 3 times, most recently from 61d748a to 248a1e3 Compare April 6, 2016 15:47
@DmitryOlshansky
Copy link
Member

LGTM then

@@ -6854,3 +6855,34 @@ pure unittest
}
}

/**
* Test if all of the characters in the array are in the ASCII set.
Copy link
Member

Choose a reason for hiding this comment

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

Probably use the string instead of the array

@JackStouffer
Copy link
Contributor Author

@DmitryOlshansky fixed

@DmitryOlshansky
Copy link
Member

And I recall @andralex wants vet all name additions to the library.

@andralex
Copy link
Member

andralex commented Apr 8, 2016

I guess something more useful would be getEncoding which returns an enum value - one of ascii, utf8, binary.

@JackStouffer
Copy link
Contributor Author

Hmm, my thoughts

  1. If we go with that route, then this should use the encoding types of std.encoding to be DRY
  2. So maybe a string overload of canEncode?

@WalterBright
Copy link
Member

I'm not too thrilled about the utility of a one liner. It would be ok if it was a loop, but it's not. It's longer for the use to read the documentation than just write the one liner.

Phobos should not be a repository of one liners. Masses of one liner functions tend to hide the meatier functions. Phobos should make it possible for users to write one liners to get their work done.

@JackStouffer
Copy link
Contributor Author

I changed the function to a helper overload of canEncode rather than a name edition.

@DmitryOlshansky
Copy link
Member

That seems better. @WalterBright?

@WalterBright
Copy link
Member

Yes, it is better. But I would rather see the one liner simply as an example in the documentation for canEncode(). It is better to teach people how to put lego blocks together than to provide trivial combinations of two lego blocks, or 3 lego blocks. It's the same reason there isn't a function to multiply x by 2.

My other complaint is the code:

s.representation.find!(x => !canEncode!E(x)).empty

is wrong. It should be:

s.byDchar.find!(x => !canEncode!E(x)).empty

The fact that nobody noticed this error suggests that we need more examples in the documentation on how to do these sorts of things. I would love to see more one liners like this as examples.

@JackStouffer
Copy link
Contributor Author

Seems reasonable.

@JackStouffer
Copy link
Contributor Author

Changed to example.

Perhaps 11356 should be marked WONTFIX?

@JackStouffer JackStouffer changed the title [Issue 11356] isASCII for strings Add Informative Example to canEncode Apr 11, 2016
@WalterBright
Copy link
Member

Auto-merge toggled on

@WalterBright
Copy link
Member

Changed to example.

Thank you. Looks good!

Perhaps 11356 should be marked WONTFIX?

Good idea. Done.

@WalterBright WalterBright merged commit 0015fd2 into dlang:master Apr 11, 2016
@JackStouffer
Copy link
Contributor Author

Thanks!

@andralex
Copy link
Member

Great. Thanks folks!

@JackStouffer JackStouffer deleted the issue11356 branch May 9, 2016 15:17
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.

6 participants