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

String prisms for primitives. #32 #33

Closed
wants to merge 3 commits into from
Closed

Conversation

NightRa
Copy link
Collaborator

@NightRa NightRa commented Mar 26, 2014

I have added String prisms for primitives.

ScalaCheck gives out some peculiar counter examples which we have to examine.

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 26, 2014

I think I figured it out.
Java parses numbers from different localizations,
but the toString on numbers return their standard string representation.

object string extends StringInstances

trait StringInstances {
def stringToBoolean: SimplePrism[String, Boolean] = SimplePrism(_.toString, s => Try(s.toBoolean).toOption)
Copy link
Member

Choose a reason for hiding this comment

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

In Bounded, you can find a safeCast method that simplifies creation of that kind of Prism

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

safeCast doesn't help in this situation, because the string's Ordering and int's boundedness don't help us, as String isn't numeric.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could add a new function for constructing prisms catching exceptions.
Where should we put it?

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 27, 2014

Shold I rebase and make a new pull request to remove unnessesary stuff from history?

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 27, 2014

What should we do about the identity failing for localized numbers?

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 27, 2014

I really don't know where to put things now with these construction functions.
If we want to put the string prisms in the B's objects, we would need to put the string prisms construction somewhere too.

  • Just need to think of a good name for a utility object to put them in.

The exception construction could be maybe put in Prism's object for more intuitive construction for users.

@julien-truffaut
Copy link
Member

No you don't need to rebase.

Good points about helper functions, let's put them in the companion object of Prism as you suggested.

I will have a read about localised numbers

@julien-truffaut
Copy link
Member

Regarding localised numbers, we should probably make stringToFloat take a Locale and use http://stackoverflow.com/questions/888088/how-do-i-convert-a-string-to-double-in-java-using-a-specific-locale.

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 29, 2014

@julien-truffaut I think it would be much cleaner to just parse only normal numbers.
Do we really need localized numbers?

@julien-truffaut
Copy link
Member

If there is a normal/standard string representation of float and double then let's use it. Otherwise, we can drop string prism for float and double for now.

@13h3r
Copy link

13h3r commented Mar 31, 2014

IMO we should keep current Java parsing behavior and possibly pass implicit Locale

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 31, 2014

@13h3r Where should we supply the implicit Locale from?
The implicit will have to be imported as it can't be resolved automatically from it's companion, which is ugly.

@NightRa NightRa changed the title String prisms for primitives. String prisms for primitives. #32 Mar 31, 2014
@NightRa
Copy link
Collaborator Author

NightRa commented Mar 31, 2014

Another option is to split the string lenses to 2 groups: localized and unlocalized.
The unlocalized will be easier to use, and the localized version will require an explicit Locale.

@13h3r
Copy link

13h3r commented Mar 31, 2014

@NightRa yes, you should import implicit, or import default locale somehow. This avoid us to push system wide parameter into every call

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 31, 2014

Having to import implicits is a usage show stopper from personal experience. (ScalaFX lifting of functions to wrappers)

@julien-truffaut
Copy link
Member

@NightRa It seems that the problem you had is not related to Locale and floating point. I launched test for stringToFloat and DoubleToFloat pass witthout problem. However, Int and Long fails for an interesting reason: some non digits characters are translated to number when you call toInt / toLong on them, see http://www.fileformat.info/info/unicode/char/aa59/index.htm and http://www.alanwood.net/unicode/cham.html

@NightRa
Copy link
Collaborator Author

NightRa commented Mar 31, 2014

@julien-truffaut This is what I was talking about.
Java parses numbers from different locales by default.

@julien-truffaut
Copy link
Member

Ah sorry I misunderstood. Then I think we should limit to basic digit numbers, we can always add more advance prisms later on

@NightRa
Copy link
Collaborator Author

NightRa commented Apr 1, 2014

@julien-truffaut What will we parse with then?

@NightRa
Copy link
Collaborator Author

NightRa commented Apr 1, 2014

We could roll out our own in part of the Int-Digit Isomorphism. #34

@julien-truffaut
Copy link
Member

let's have a look around if it exists some parser that do the job. Otherwise yes, we can roll our own.

NightRa added a commit to NightRa/Monocle that referenced this pull request Apr 13, 2014
@NightRa
Copy link
Collaborator Author

NightRa commented Apr 13, 2014

Changing branch.
#46 is the new pull request.

@NightRa NightRa closed this Apr 13, 2014
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

3 participants