Hi Monte,

Thanks for the pull request, before accepting I've got just a few comments (mostly) about code style at the moment (we're still finalizing our coding standards doc for public release!). If you could review, and update the request appropriately that would be great.

First a couple of stylistic comments:
- we prefer braces to always be on separate lines, rather than bunched up.
- things like p_execution_point and t_execution_status is a bit verbose, there's no problem here using p_ep or ep for p_execution_point, and t_stat for t_execution_status. These two concepts are so widely used that these abbreviated forms make things a lot more readable.
- getobj() doesn't really return the parent_id exactly, its more part_id or card_id.

Secondly, I suggest restructuring the code a bit to use a cascade approach, rather than nested if conditions. This will reduce the repeated code. So, using t_stat as the 'success' variable, you can do something like:

t_stat = ES_NORMAL;

uint4 t_part_id;
MCImage *t_relative_object;
if (t_stat == ES_NORMAL)
    t_stat = m_relative_object -> getobj(ep, t_relative_object, t_part_id, True);

if (t_stat == ES_NORMAL)
    t_stat = m_id_or_name -> eval(ep);

MCImage *t_found_image;
t_found_image = nil;
if (t_stat == ES_NORMAL)
{
    if (m_is_id)
    {
        if (ep . ton() == ES_ERROR) // Note that you need to check its a number before converting.
        {
            MCeerror -> add(...) // Need to add an appropriate error constant
            return ES_ERROR;
        }

        t_found_image = t_relative_object -> resolveimageid(ep . getuint4());
    }
    else
        t_found_image = t_relative_object -> resolveimagename(ep . getsvalue());

    if (t_found_image != nil)
        t_stat = t_found_image -> getprop(0, P_LONG_ID, ep, False);
    else
        ep . clear();
}

if (t_stat == ES_NORMAL)
    t_stat = m_it -> set(ep);

return t_stat;

Notice here also the need to check the type in the EP before converting and throwing an error if its not of the correct type. You can add a suitable error constant to the executionerrors.h file - or if you find a generic Not a Number one that seems suitable, just use that.

Also notice stylistically that the local variables are defined just before they are used which makes it a bit clearer how the variables are used.

Any further questions just ask,

Mark.

Thanks Mark, ill look make the changes ASAP... Guess I'm used to a verbose language for some reason...

M E R Goulding
Software development services

mergExt - There's an external for that!

On 15/04/2013, at 11:14 PM, runrevmark [email protected] wrote:

Hi Monte,

Thanks for the pull request, before accepting I've got just a few comments (mostly) about code style at the moment (we're still finalizing our coding standards doc for public release!). If you could review, and update the request appropriately that would be great.

First a couple of stylistic comments:

• we prefer braces to always be on separate lines, rather than bunched up.
• things like p_execution_point and t_execution_status is a bit verbose, there's no problem here using p_ep or ep for p_execution_point, and t_stat for t_execution_status. These two concepts are so widely used that these abbreviated forms make things a lot more readable.
• getobj() doesn't really return the parent_id exactly, its more part_id or card_id.

Secondly, I suggest restructuring the code a bit to use a cascade approach, rather than nested if conditions. This will reduce the repeated code. So, using t_stat as the 'success' variable, you can do something like:

t_stat = ES_NORMAL;

uint4 t_part_id;
MCImage *t_relative_object;
if (t_stat == ES_NORMAL)
t_stat = m_relative_object -> getobj(ep, t_relative_object, t_part_id, True);

if (t_stat == ES_NORMAL)
t_stat = m_id_or_name -> eval(ep);

MCImage *t_found_image;
t_found_image = nil;
if (t_stat == ES_NORMAL)
{
if (m_is_id)
{
if (ep . ton() == ES_ERROR) // Note that you need to check its a number before converting.
{
MCeerror -> add(...) // Need to add an appropriate error constant
return ES_ERROR;
}

    t_found_image = t_relative_object -> resolveimageid(ep . getuint4());
}
else
    t_found_image = t_relative_object -> resolveimagename(ep . getsvalue());

if (t_found_image != nil)
    t_stat = t_found_image -> getprop(0, P_LONG_ID, ep, False);
else
    ep . clear();

}

if (t_stat == ES_NORMAL)
t_stat = m_it -> set(ep);

return t_stat;
Notice here also the need to check the type in the EP before converting and throwing an error if its not of the correct type. You can add a suitable error constant to the executionerrors.h file - or if you find a generic Not a Number one that seems suitable, just use that.

Also notice stylistically that the local variables are defined just before they are used which makes it a bit clearer how the variables are used.

Any further questions just ask,

Mark.

—
Reply to this email directly or view it on GitHub.

On second thoughts I think I'll wait for the style docs because I'm just wasting both our time if I don't understand your requirements. Also I'll need to rebase on release-6.0.1 to test any changes so you probably won't be able to merge it in until after that...

It's up to you of course, but the only stylistic comments were really about braces and shortening the vars for ep and stat.

The rest was more about a better way to structure the code to make it clearer and solve the code duplication issue.

The 'success cascade' approach is one which we use everywhere for new code as its a pattern that works really well.

I'm happy to include this in 6.0.1 since its in the internal module and required no other code changes. Indeed, if you want I'll restructure and integrate so you can see a specific example of style? (I can then use it as an example of the success cascade stuff in the style doc - e.g. before and after)

Sent from my iPhone

On 15 Apr 2013, at 22:53, Monte Goulding [email protected] wrote:

On second thoughts I think I'll wait for the style docs because I'm just wasting both our time if I don't understand your requirements. Also I'll need to rebase on release-6.0.1 to test any changes so you probably won't be able to merge it in until after that...

—
Reply to this email directly or view it on GitHub.

Oh I'm all for descriptive variable names and you'll find places in the engine where I used similar things for ep and stat. However, pragmatically those are concepts which are used throughout so after a while I saw the sense in abbreviating them (as the engine src already did).

Sent from my iPhone

On 15 Apr 2013, at 21:16, Monte Goulding [email protected] wrote:

Thanks Mark, ill look make the changes ASAP... Guess I'm used to a verbose language for some reason...

M E R Goulding
Software development services

mergExt - There's an external for that!

On 15/04/2013, at 11:14 PM, runrevmark [email protected] wrote:

Hi Monte,

Thanks for the pull request, before accepting I've got just a few comments (mostly) about code style at the moment (we're still finalizing our coding standards doc for public release!). If you could review, and update the request appropriately that would be great.

First a couple of stylistic comments:

• we prefer braces to always be on separate lines, rather than bunched up.
• things like p_execution_point and t_execution_status is a bit verbose, there's no problem here using p_ep or ep for p_execution_point, and t_stat for t_execution_status. These two concepts are so widely used that these abbreviated forms make things a lot more readable.
• getobj() doesn't really return the parent_id exactly, its more part_id or card_id.

Secondly, I suggest restructuring the code a bit to use a cascade approach, rather than nested if conditions. This will reduce the repeated code. So, using t_stat as the 'success' variable, you can do something like:

t_stat = ES_NORMAL;

uint4 t_part_id;
MCImage *t_relative_object;
if (t_stat == ES_NORMAL)
t_stat = m_relative_object -> getobj(ep, t_relative_object, t_part_id, True);

if (t_stat == ES_NORMAL)
t_stat = m_id_or_name -> eval(ep);

MCImage *t_found_image;
t_found_image = nil;
if (t_stat == ES_NORMAL)
{
if (m_is_id)
{
if (ep . ton() == ES_ERROR) // Note that you need to check its a number before converting.
{
MCeerror -> add(...) // Need to add an appropriate error constant
return ES_ERROR;
}

t_found_image = t_relative_object -> resolveimageid(ep . getuint4());
}
else
t_found_image = t_relative_object -> resolveimagename(ep . getsvalue());

if (t_found_image != nil)
t_stat = t_found_image -> getprop(0, P_LONG_ID, ep, False);
else
ep . clear();
}

if (t_stat == ES_NORMAL)
t_stat = m_it -> set(ep);

return t_stat;
Notice here also the need to check the type in the EP before converting and throwing an error if its not of the correct type. You can add a suitable error constant to the executionerrors.h file - or if you find a generic Not a Number one that seems suitable, just use that.

Also notice stylistically that the local variables are defined just before they are used which makes it a bit clearer how the variables are used.

Any further questions just ask,

Mark.

—
Reply to this email directly or view it on GitHub.
—
Reply to this email directly or view it on GitHub.

On 16/04/2013, at 6:01 PM, runrevmark [email protected] wrote:

I'm happy to include this in 6.0.1 since its in the internal module and required no other code changes. Indeed, if you want I'll restructure and integrate so you can see a specific example of style? (I can then use it as an example of the success cascade stuff in the style doc - e.g. before and after)

I'm doing it right now ;-) ... didn't wait long did I?

I should be able to push the change once the kids go to bed but I've had to merge it with release-6.0.1 to get your config changes so I could test. If you remember I had to jump through a few hoops because the lowest sdk I have is 10.6. So the pull request will now be merged already with your commit today where you updated the submodules. It doesn't really make much difference I don't think as you can always decide to fast forward to it or delete it after you fetch the branch.

Unfortunately the merge on release-6.0.1 was required for the config changes

Hi Monte,

I made a couple of minor changes before integrating...

Since the class is in 'internal_development.cpp' at the moment, I've renamed it MCInternalResolveImage (this was just for consistency - this part of the ide engine needs cleaning up at some point!).

There's no need to test pointers for nil before 'delete' - delete will ignore nil pointers.

Using a bool bit-field (of length 1) is preferable to Boolean... They take up the same amount of space for individual ones, but if there are lots next to each other, they only take up single bits (rounded up to the next padding boundary).

Apart from that, your first contribution will be in release-6.0.1 - thanks very much for your efforts :)

Mark.

Hi Mark

I'll take note of the bool thing. The nil pointer thing I originally didn't have the tests but then I found some (what looked like newer) code in the engine that did have the tests so I thought you would want it...

Not sure why I named the class IDE instead of Internal... no excuse for doing something different to the rest of the classes in the file there...

It seems like you've had to do more work here than if you did this yourself but hopefully that will change over time as people come to understand your coding standards.

Just a note about the branch names etc.... It looks like you created a new branch and copied my code into it instead of just fetching my branch. I think that will negate the role of git blame if you do that too often. Also obviously you don't like the feature/featurename branch name convention which is standard git-flow stuff. SourceTree (not sure about other tools) will even display those branches in folders (I think GIT uses folders internally for the branch pointers) which can keep things clean... folders for release,feature,bugfix.... perhaps hotfix for a change requiring a bump in GM number. I personally like the convention and hope you will consider it.

Cheers

Monte

Hi Monte,

It wasn't a problem - it's all a good learning exercise... As well as helping to flesh out what we need to say in regards to 'coding standards'.

Hmmm - I must have done something slightly wrong when I merged in your branch admittedly as it doesn't seem to have preserved any commit log messages from you. Apologies for that - still new at the whole git thing. I thought I'd done what I'd done with all the other pul requests - created a branch, pulled in your changes, pushed the branch then merge '--no-ff''d it into the release-6.0.1 branch...

I've been following the naming convention suggested by gitflow in 'A successful git branching model'... Which was 'feature-*' for features. I must confess I did notice the folder convention and wondered if that might be a better way to go, however at that point I didn't have enough time to play about with it and see how it worked so stuck with what I knew. Certainly will look into it though - especially if there are some GUI tools that help manage such things and display stuff better.

Warmest Regards,

Mark.

---

From: Monte Goulding [email protected]
To: runrev/livecode [email protected]
Cc: runrevmark [email protected]
Sent: Wednesday, 17 April 2013, 23:03
Subject: Re: [livecode] added _internal resolve image [id] relative to (#5)

Hi Mark
I'll take note of the bool thing. The nil pointer thing I originally didn't have the tests but then I found some (what looked like newer) code in the engine that did have the tests so I thought you would want it...
Not sure why I named the class IDE instead of Internal... no excuse for doing something different to the rest of the classes in the file there...
It seems like you've had to do more work here than if you did this yourself but hopefully that will change over time as people come to understand your coding standards.
Just a note about the branch names etc.... It looks like you created a new branch and copied my code into it instead of just fetching my branch. I think that will negate the role of git blame if you do that too often. Also obviously you don't like the feature/featurename branch name convention which is standard git-flow stuff. SourceTree (not sure about other tools) will even display those branches in folders (I think GIT uses folders internally for the branch pointers) which can keep things clean... folders for release,feature,bugfix.... perhaps hotfix for a change requiring a bump in GM number. I personally like the convention and hope you will consider it.
Cheers
Monte
—
Reply to this email directly or view it on GitHub.

Hmm... wonder if that's what happens if the branch names are different? I guess up until now your branch names and the pull request branch name have been the same.

I see you are right that Vincent Driessen's original branching model doesn't use slash so I'm obviously wrong that it's a standard git flow thing. I wonder if that's just a SourceTree thing. SourceTree has a menu for git flow by the way... start feature etc... it's pretty handy if you want to have a UI view of the repo... I tend to jump between SourceTree and command line depending on the situation.

I'll see if I can look back at what I did in this case - it should be in my bash history... I had thought the 'blame' was attached to the commit objects themselves, and thus when you pull in changes from another branch they come too (since git - I believe - preserves commit objects as far as it can).

I've been limiting myself to command-line for the moment - so I can get a handle on how everything works... Its useful to know the low-level details when something goes wrong!

---

From: Monte Goulding [email protected]
To: runrev/livecode [email protected]
Cc: runrevmark [email protected]
Sent: Thursday, 18 April 2013, 9:04
Subject: Re: [livecode] added _internal resolve image [id] relative to (#5)

Hmm... wonder if that's what happens if the branch names are different? I guess up until now your branch names and the pull request branch name have been the same.
I see you are right that Vincent Driessen's original branching model doesn't use slash so I'm obviously wrong that it's a standard git flow thing. I wonder if that's just a SourceTree thing. SourceTree has a menu for git flow by the way... start feature etc... it's pretty handy if you want to have a UI view of the repo... I tend to jump between SourceTree and command line depending on the situation.
—
Reply to this email directly or view it on GitHub.

I'll see if I can look back at what I did in this case - it should be in my bash history... I had thought the 'blame' was attached to the commit objects themselves, and thus when you pull in changes from another branch they come too (since git - I believe - preserves commit objects as far as it can).

Yeah... I can't actually understand how it happened other than if you didn't actually pull my branch and just pasted my code into your branch... but it doesn't sound like you did that. Would be good to work it out.

I've been limiting myself to command-line for the moment - so I can get a handle on how everything works... Its useful to know the low-level details when something goes wrong!

Good idea... git has low levels on it's low levels ;-)

The blame stuff is still all there - if you look at internal_development.cpp in release-6.0.1 on github, it shows up fine so nothing has been lost :)

---

From: Monte Goulding [email protected]
To: runrev/livecode [email protected]
Cc: runrevmark [email protected]
Sent: Thursday, 18 April 2013, 9:04
Subject: Re: [livecode] added _internal resolve image [id] relative to (#5)

Hmm... wonder if that's what happens if the branch names are different? I guess up until now your branch names and the pull request branch name have been the same.
I see you are right that Vincent Driessen's original branching model doesn't use slash so I'm obviously wrong that it's a standard git flow thing. I wonder if that's just a SourceTree thing. SourceTree has a menu for git flow by the way... start feature etc... it's pretty handy if you want to have a UI view of the repo... I tend to jump between SourceTree and command line depending on the situation.
—
Reply to this email directly or view it on GitHub.

The blame stuff is still all there - if you look at internal_development.cpp in release-6.0.1 on github, it shows up fine so nothing has been lost :)

You're right, so... I think the only problem was the branch names were different so I think GIT decided my branch wasn't merged even though it was merged into your branch which was merged... So either we need to agree on branch names before branches are pushed to our remotes to issue the pull or you need to just accept whatever branch names you get sent. You probably want to delete the branch after they are merged with your long running branches anyway because there's no need for them to hang around. The commit messages should note issue numbers etc...

Something seems wonky with you having to create the branch first anyway. Can't you just fetch then merge?

Ah if you are referring to the listing in github saying it isn't merged... That's because they were formed from master - so they only come up as 'unmerged' when they merge back in there (all my bugfix- branches show as unmerged at the moment)... I'm assuming that will happen when release-6.0.1 gets merged into master (when its ready).

Well, you have to create a local branch to pull in changes... The process is:
  create local branch
  pull in branch from (your) remote
  -- check everything is fine
  
  checkout target for merging (e.g. release-6.0.1)
  merge local branch
  push

I've been pushing and keeping the branches in the livecode repo so that they can be updated and then merged if changes need to happen to the branch (e.g. a bugfix not being quite right, or a bug in a feature). I did this with the 'feature-overhaul_commercial' and it worked well - Ian could pull it out add stuff to it, push back then do a pull request.

The branches associated with any particular release will be deleted when that release goes out since we can assume (hopefully) they are stable :)

---

From: Monte Goulding [email protected]
To: runrev/livecode [email protected]
Cc: runrevmark [email protected]
Sent: Thursday, 18 April 2013, 9:45
Subject: Re: [livecode] added _internal resolve image [id] relative to (#5)

The blame stuff is still all there - if you look at internal_development.cpp in release-6.0.1 on github, it shows up fine so nothing has been lost :)

You're right, so... I think the only problem was the branch names were different so I think GIT decided my branch wasn't merged even though it was merged into your branch which was merged... So either we need to agree on branch names before branches are pushed to our remotes to issue the pull or you need to just accept whatever branch names you get sent. You probably want to delete the branch after they are merged with your long running branches anyway because there's no need for them to hang around. The commit messages should note issue numbers etc...

Something seems wonky with you having to create the branch first anyway. Can't you just fetch then merge?
—
Reply to this email directly or view it on GitHub.

Well, you have to create a local branch to pull in changes...

The following might or might not be simpler...

git fetch montegoulding
git checkout -b feature-ide_resolve_image montegoulding/feature/_internalResolveImage
-- test
git checkout release-6.0.1
git merge feature-ide_resolve_image

You could also simplify this using

git checkout release-6.0.1
git fetch montegoulding
git merge montegoulding/feature/_internalResolveImage
-- test

If the tests fail in the second case though you would need to reset --hard