trivial bug fixes (SI-6654 and SI-7005)

[John Sullivan]

Good morning!

I was playing around with forking the project and going through the build and test cycle, and I came up with fixes for a couple of trivial bugs:

SI-6654 return of filterKeys is not serializable [1]
SI-7005 Map#mapValues is not serializable [2]

I can make a pull request for these at any time. If you want to preview see [3]. I know these issues are both related to SI-4776 [4], but I don't see any harm in putting in these trivial fixes first. Sorry for not discussing this on the internals mailing list first. It was really just an exercise to learn the build a bit.

Best,
John Sullivan

P.S. I signed the CLA a couple years back, but let me know if you want me to sign again

[1] https://issues.scala-lang.org/browse/SI-6654
[2] https://issues.scala-lang.org/browse/SI-7005
[3] https://github.com/sullivan-/scala/compare/master...SI-6654-SI-7005
[4] https://issues.scala-lang.org/browse/SI-4776

[Paolo G. Giarrusso]

On Saturday, March 23, 2013 1:53:14 AM UTC+9, John Sullivan wrote:

Good morning!

Hi, and welcome!

 

I was playing around with forking the project and going through the build and test cycle, and I came up with fixes for a couple of trivial bugs:

SI-6654 return of filterKeys is not serializable [1]
SI-7005 Map#mapValues is not serializable [2]

I can make a pull request for these at any time. If you want to preview see [3]. I know these issues are both related to SI-4776 [4], but I don't see any harm in putting in these trivial fixes first.

Sounds quite sensible.

Don't let silence stop you - if nobody answers, we might very well want your fix. So do send a pull request.

Don't forget to name a reviewer and to remind him - I'd suggest @axel22 (for collections) and @rkuhn, since he argued for fixing SI-4776 instead, so maybe he figured this is a bad idea and can explain why - but I guess not.

Otherwise, your request seems already good to me, including test cases and commit conventions, so I assume you already read https://github.com/scala/scala/blob/master/CONTRIBUTING.md.

I think you should target master because your change doesn't seem *forward* binary compatible (see previous discussions on this mailing list): you make some protected classes newly extend Serializable.

 

Sorry for not discussing this on the internals mailing list first. It was really just an exercise to learn the build a bit.

I'm just a young contributor, but I observe that minor pull requests are not discussed on scala-internals, while it happens mostly for more controversial changes.

Also, you are not required to discuss changes *before* doing them. So nothing to apologize, and please keep on hacking! Wanna try SI-4776?

[Aleksandar Prokopec]

Hi, John!

Those changes look ok to me. Feel free to submit a pull request and add me as a reviewer.

Thanks,
Alex

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

-- 
Aleksandar Prokopec,
Doctoral Assistant
LAMP, IC, EPFL
http://people.epfl.ch/aleksandar.prokopec

[John Sullivan]

Hi Alex!

How have you been? I'm looking forward to seeing you at Scala Days in June. I made the pull request, and listed you as a reviewer in the commit log, so you should get some kind of auto notification, but just in case, the pull request is here:

https://github.com/scala/scala/pull/2308

Best, John

--
You received this message because you are subscribed to a topic in the Google Groups "scala-internals" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/scala-internals/oso5yrKV5ds/unsubscribe?hl=en.
To unsubscribe from this group and all its topics, send an email to scala-interna...@googlegroups.com.

[John Sullivan]

Hi Paolo,

Thanks for the welcome!

Don't let silence stop you - if nobody answers, we might very well want your fix. So do send a pull request.

Thanks, I appreciate it. I knew that I had sent out that message right at the end of the week, and probably well into the weekend in Europe, so I was willing to wait until Monday before prodding.

 

Don't forget to name a reviewer and to remind him - I'd suggest @axel22 (for collections) and @rkuhn, since he argued for fixing SI-4776 instead, so maybe he figured this is a bad idea and can explain why - but I guess not.

Thanks, both Alex and Roland are listed as reviewers in the commit log.

 

Otherwise, your request seems already good to me, including test cases and commit conventions, so I assume you already read https://github.com/scala/scala/blob/master/CONTRIBUTING.md.

And I taught myself how to rebase in the process!

 

I think you should target master because your change doesn't seem *forward* binary compatible (see previous discussions on this mailing list): you make some protected classes newly extend Serializable.

I'll have to try to figure out what forward binary compatibility is about.

 

Wanna try SI-4776?

That looks like a fun exercise and would really force me to learn more about how Scala collections are implemented. But I see it is already assigned to Josh. I'm not sure if his being assigned to the ticket is really of any significance, but I wouldn't want to step on anyone's toes.

This one also looks like a fun exercise: SI-6909 - identity functions should be macros

I need to take care of a couple other things before I would be able to look at either of them. Maybe in a couple weeks.

Best, John

[Aleksandar Prokopec]

Hey, John!

I'm doing pretty well, thank you - I hope you are ok as well!
Yes, I saw the pull request, and commented - current changes look good to me.

I think that should cover all the collection wrappers we have. Those in the convert package are all case classes, so that should be fine too.
As for views - those are not serializable, but I believe that is ok.

Cheers,
Alex