first commit!

[Martin Charlesworth]

Hi Graham, I see you allowed (pulled? posted? patched? what's the lingo for this?) my first pull request, that's great! 

So how shall I proceed? I've run the debugger & I see how it finds its way to CollectionTypeWrappedInUnmodifiableIdiomChecker & I see it's a hard-coded set of idioms and classes that it considers immutable. So do you have a specific idea of how you'd like me to proceed, or shall I just have a go & see what happens?! cheers,

martin

[Graham Allan]

Really? I didn't do anything and I don't see any commits to my repo:

https://github.com/MutabilityDetector/MutabilityDetector/commits/master

Ttechnically I think Mani would have been able to merge a pull request, but he hasn't. What makes you think a pull request has been merged? I haven't received any pull requests*, did you create one through the GitHub web interface, or some other way? Also, what's your GitHub username?

We'll work on getting your first pull request complete, then move on to attacking the problem :-)

Cheers!

Graham A

* this is the lingo for merging in your changes into the main repository.

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

[Martin Charlesworth]

Ah. I must have been looking at this https://github.com/grommitz/MutabilityDetector, but of course that's my cloned version not the master. I just did "git pull" on the command line, I thought that would do it but maybe not. You think it's better to use the web ui to issue a pull request?

[Graham Allan]

I think there's some very similar terms used in git (and GitHub) that make it confusing to talk about. As far as I know here's the correct terms for some of these things.

You have a 'fork' of the main Mutability Detector repo, that's the repository at https://github.com/grommitz/MutabilityDetector

You have a branch called 'master' in your fork, hosted on GitHub. I also have a branch called 'master' in the main repository (the main repository is also called 'upstream'). It's like trunk from SVN, except we both have one, because you forked the repository. Our respective repositories can have different branches, like if you want a branch for a released version. I don't think there's any need to do that on this project though.

You have a 'clone' of the of your fork on your development machine. That clone contains all the branches from the repository (i.e. GitHub, sometimes referred to as 'origin' in git output). I have a clone of the upstream repository on my laptop and main development machine.

You do 'git pull' when you want to update your clone. It's like SVN up. You only really need to worry about it with GitHub if you have other people committing to the same repository, or you push from different machines. It's like if I have checked out an SVN repository on two machines, when I commit ('svn ci'/'git push') from one machine, I need to update ('svn up'/'git pull') when I start work on the other machine, to keep the code up to date with the central repository.

Now is where it gets tricky for me to explain, as I'm not entirely sure of the terms or the process.

When you want code changes from your fork merged upstream, I think you need to 'rebase' from upstream, before issuing the pull request. More details on how to rebase here: http://stackoverflow.com/questions/7244321/how-to-update-github-forked-repository

Once the branch you're working on in your fork looks like 'up to date upstream code' + 'your single changeset', then you issue a pull request. This helps to make it a 'clean' merge. I would do that through the GitHub web UI, I'm not sure if there's a better 

*phew*

Does that help? As you say, there's a lot of 'lingo', and when it comes to how to use git, I have to hit the web if I'm doing anything that's not git pull, git commit or git push, because I don't remember any of the other commands :-)

If that all makes sense, can you issue a pull request with your changeset?

Cheers!

Graham A

On 24 September 2013 17:09, Martin Charlesworth <martincha...@gmail.com> wrote:

Ah. I must have been looking at this https://github.com/grommitz/MutabilityDetector, but of course that's my cloned version not the master. I just did "git pull" on the command line, I thought that would do it but maybe not. You think it's better to use the web ui to issue a pull request?

[Graham Allan]

Ah, while I was typing that manifesto you issued a pull request! Merged into upstream. Good stuff.

In terms of fixing the issue, I think it would be useful to break it down into smaller chunks. How about this: work on the code that would allow an end user to configure the checker to allow this code to pass:

public final class ImmutableClass {

  private final List<String> myStrings;

  public ImmutableClass(List<String> source) {

    this.myStrings = Collections.unmodifiableList(com.google.common.collect.Lists.newArrayList(source));

  }

}

This will currently fail because it doesn't recognise Lists.newArrayList as a 'copying' method. Eventually we want to support Guava out of the box, but imagine if a user had they're own custom collection which had a constructor which took a source collection, and copied all the elements into a new collection, they would want to be able to specify that "new MyCustomCollection(Collection source)" was a valid 'copy' method. Thus it would need to be configured and recognised by Mutability Detector. That involves creating the API for the configuration, and threading it all the way through to the CollectionTypeWrappedInUnmodifiableIdiomChecker.

Happy doing that?

~ Graham A

[Martin Charlesworth]

Thanks for the git manifesto :-) That's pretty much what I thought tho I thought my fork only lived on my machine, i didn't realise it had a "mirror" on github which automagically updates.

The task sounds reasonable, leave it with me! I'll probably come back with questions once I'm more au fait with what I need to do. cheers,

martin

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsub...@googlegroups.com.

[Martin Charlesworth]

So I've committed something to my fork which works. But it's not yet done "properly". It reaches inside CollectionTypeWrappedInUnmodifiableIdiomChecker and adds the allowed copy methods to a new list in there. The list is static so it remains for later runs which we wouldn't want, so now I have to work out how to thread the information through from Configuration into where it is required.

Tomorrow I'll try it with 1 or 2 GS collections. When it's working I don't expect there to really be anything specific that has to be done for other frameworks, tho it'd be fun to collaborate on the next task :-)

martin

On Tuesday, 24 September 2013 18:06:57 UTC+1, Graham Allan wrote:

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsub...@googlegroups.com.

[Graham Allan]

Brilliant!

Just to set expectations about what I'm looking for in pull requests, I would like:

• comprehensive tests (I will check coverage with code review and also the Emma coverage tool)
• a fluent + documented API around the classes a user would use to configure analysis. Once you get into code that end users aren't expecting to see I expect to see next to no JavaDoc. This is because I believe in combining self-documenting code and also tests as living documentation. I draw parallels between whitebox/blackbox testing when it comes to JavaDoc.
• inversion of control/dependency injection where it makes sense. You already mentioned the downsides of a static list, I see dependency injection as the way to avoid that. Note I mean manual dependency injection, not e.g. Guice. Should be plenty of examples in the codebase.
• objects being immutable unless there's a very good reason not to (as you may expect)

That is to say, any code pushed upstream should have these properties (and probably more I haven't listed), but don't feel you have to match them all before seeking feedback/issuing a pull request. No doubt there will be a bit of back and forth discussion as we iron out details.

Hopefully that all sounds okay to you :)

~ Graham A

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.

[Martin Charlesworth]

On Friday, 27 September 2013 09:35:31 UTC+1, Graham Allan wrote:

Brilliant!

Just to set expectations about what I'm looking for in pull requests, I would like:

• comprehensive tests (I will check coverage with code review and also the Emma coverage tool)

no problem, I use eclEmma when I'm writing tests too

 

• a fluent + documented API around the classes a user would use to configure analysis. Once you get into code that end users aren't expecting to see I expect to see next to no JavaDoc. This is because I believe in combining self-documenting code and also tests as living documentation. I draw parallels between whitebox/blackbox testing when it comes to JavaDoc.

agree with keeping comments/javadoc minimal, that said I find a one liner at the top of a class can be helpful even if you try really hard to find good names

 

• inversion of control/dependency injection where it makes sense. You already mentioned the downsides of a static list, I see dependency injection as the way to avoid that. Note I mean manual dependency injection, not e.g. Guice. Should be plenty of examples in the codebase.

yes, while i was running today I realised that's what I meant about the configuration - that the dependency is the wrong way round

 

• objects being immutable unless there's a very good reason not to (as you may expect)

goes without saying in this project?!!

 

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsubscribe...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Graham Allan]

> goes without saying in this project?!!

:-D

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.

[Graham Allan]

Hey Martin,

Any updates? Anything blocking you? As I also mentioned to Marc Gomez in another thread, if you haven't found the time that's no problem.

Cheers,

Graham A

On Friday, 27 September 2013 00:26:37 UTC+1, Martin Charlesworth wrote:

So I've committed something to my fork which works. But it's not yet done "properly". It reaches inside CollectionTypeWrappedInUnmodifiableIdiomChecker and adds the allowed copy methods to a new list in there. The list is static so it remains Not being able to find the timefor later runs which we wouldn't want, so now I have to work out how to thread the information through from Configuration into where it is required.

[Martin Charlesworth]

Hi! Sorry its been a hectic week my wife hurt her back about 10 days so I've been on double kids duties, so haven't had the time or energy for programming! But I'm back, I hope :-) I think I've more or less finished my task but I need to tidy everything up, check the coverage etc before I issue the pull request. I'm still struggling with the git way though... is there a way to see all the changes I've made compared to the master repo either on my local copy or in my fork? In SVN world I'd compare all my mods before committing, is there an equivalent with git pull? And I've made several intermediate commits on my fork, so if/when they get pulled into the master do you get all the intermediate commits as well or does git roll it all up into one big changeset?

martin

[Graham Allan]

Hi Martin,

Sorry its been a hectic week my wife hurt her back about 10 days so I've been on double kids duties, so haven't had the time or energy for programming!

If you haven't been able to find the time, you should definitely not feel that you have to explain yourself, and certainly not feel the need to apologise. We all lives to live :) Hope your wife gets better soon.

is there a way to see all the changes I've made compared to the master repo either on my local copy or in my fork?

I believe what you want to do is diff your entire branch against another. What happens when you run 'git diff' is that you're comparing your local changes to your local up to date branch. The only difference is you don't want to compare against "origin/master" (which is your fork) but compare against "main_mutability_dectector_repository/master". You have to configure git to tell it where to find "main_mutability_detector_repo". I think this stack overflow question might help:

http://stackoverflow.com/questions/3792989/how-to-view-diff-of-a-forked-github-project

I believe when you issue a pull request, it contains _all_ the commits from your repository, combined into a single pull request.

Let me know if you have any questions.

~ Graham A

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.

[Martin Charlesworth]

On Sunday, 13 October 2013 16:17:42 UTC+1, Graham Allan wrote:

Hi Martin,

Sorry its been a hectic week my wife hurt her back about 10 days so I've been on double kids duties, so haven't had the time or energy for programming!

If you haven't been able to find the time, you should definitely not feel that you have to explain yourself, and certainly not feel the need to apologise. We all lives to live :) Hope your wife gets better soon.

Cheers. Just letting you guys know where I'm at, that's all! 

is there a way to see all the changes I've made compared to the master repo either on my local copy or in my fork?

I believe what you want to do is diff your entire branch against another. What happens when you run 'git diff' is that you're comparing your local changes to your local up to date branch. The only difference is you don't want to compare against "origin/master" (which is your fork) but compare against "main_mutability_dectector_repository/master". You have to configure git to tell it where to find "main_mutability_detector_repo". I think this stack overflow question might help:

http://stackoverflow.com/questions/3792989/how-to-view-diff-of-a-forked-github-project

That looks useful, I'll give it a try.

 

I believe when you issue a pull request, it contains _all_ the commits from your repository, combined into a single pull request.

That makes sense.

 

martin

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsubscribe...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.