moodle-checklist
moodle-checklist copied to clipboard
davosmith
Hi Davo, checklist fork ready
I did all the modifications you suggested, except lang files sorting, but sure it will be done when that fork will be stabilized. For Mahara stuff I'll set a new branch... I'll be in vacations next week. Jean.
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module.
I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists.
The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed?
I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means).
"follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git).
Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there.
Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on.
Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed.
I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this.
Hi Davo,
I had never used GIT. Surely the way I did that fork is not the righ one.
I shall delete all that fork and redo a brand new one. I'll try to do a commit per new feature (but surely not one for each line of code !;>))
New features type 1: 1.1 - outcomes as list of items (affect import / export) 1.2 - outcomes validation made in any Moodle activity pushed to Checklist+ (affect cron job)
New features type 2: 2.1 - prove of practice attached to each item (affect table structure) 2.2 - Mahara connection (selected links / export data) (uses portfolio)
Please feel free to give me your keen advice, for wich I'll thank you much.
Regards. Jean.
P.S.
I have presented the CheckList+ to French MoodleMoot 2012. http://moodlemoot2012.unimes.fr/ http://moodlemoot2012.unimes.fr/course/view.php?id=50
A lot of people interested by that evolution...
JF.
Le 07/07/2012 19:28, Davo Smith a écrit :
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module.
I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding. OK. I'll try that.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists.
The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed?
I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means).
"follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git).
Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there.
Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on.
Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed.
I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this. OK. That's correct.
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6824864
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5118 - Date: 08/07/2012
Jean FRUITET - St Sébastien / Loire
Yes - one commit per new feature / bug fix would be great - one commit per line would be terrible!
I use the command line version of git and this is how I would proceed.
( git config --global color.ui auto // makes the output a lot prettier )
- git add remote davosmith git://github.com/davosmith/moodle-checklist.git // This adds my official repo as a source for your repo
- git fetch davosmith // Get all the latest code from my repo
- git checkout davosmith/master // Check out my master branch
- git checkout -b mynewbranch // Create a new branch, off my master branch, to put your code in (give it a better name than that!)
- Copy & paste in the code for the first feature / fix (try to avoid adding trailing whitespace or adjusting the layout of any of the existing code)
- git diff // to check exactly what you have changed - I can also recommend gitk (if you have it installed) to check
- git add FILENAME // for each file you have changed
- (optional) use gitk again, to double-check all the files / changes are queued up correctly; you can also use 'git status' to check the queued files as well
- git commit -m "Brief description of the feature that is being added" // If it needs more explanation, leave out the -m "Brief description ...." and type into the text editor that appears a brief description, a blank line, then a more detailed explanation.
- Go back to step 5 until all changes checked in
- git push origin mynewbranch:mynewbranch // This pushes 'mynewbranch' to github and creates a new branch there called 'mynewbranch' (substitute the branch name you used in step 4)
- Go onto github and put in a pull request from 'mynewbranch'
There are some alternatives to 'git add FILENAME' - you can also use 'git add *.php' (to add all PHP files in the current folder) or 'git add db' (to add all files in the db folder + sub folders) or 'git add .' (to add everything in the current folder and subfolders). You can also, skip the 'git add' altogether and call 'git commit -am "brief description"' if you know exactly what you have changed, as this adds and commits all the previously known files (but does not add any new files). Use this last option with great care (I have often committed changes that I didn't want, or missed new files with this version).
One thing you might want to do BEFORE step 11, is to do another 'git fetch davosmith' to see if I have changed anything (unlikely) and then a 'git rebase davosmith/master' to pull in my latest changes and then re-run your changes over the top. Do not do this after you have pushed to github, as it won't work (without a --force parameter).
I hope that helps you out a bit with Git - feel free to ask a question if you get stuck ( [email protected] )
Davo
On Mon, Jul 9, 2012 at 5:07 PM, Jean FRUITET < [email protected]
wrote:
Hi Davo,
I had never used GIT. Surely the way I did that fork is not the righ one.
I shall delete all that fork and redo a brand new one. I'll try to do a commit per new feature (but surely not one for each line of code !;>))
New features type 1: 1.1 - outcomes as list of items (affect import / export) 1.2 - outcomes validation made in any Moodle activity pushed to Checklist+ (affect cron job)
New features type 2: 2.1 - prove of practice attached to each item (affect table structure) 2.2 - Mahara connection (selected links / export data) (uses portfolio)
Please feel free to give me your keen advice, for wich I'll thank you much.
Regards. Jean.
P.S.
I have presented the CheckList+ to French MoodleMoot 2012. http://moodlemoot2012.unimes.fr/ http://moodlemoot2012.unimes.fr/course/view.php?id=50
A lot of people interested by that evolution...
JF.
Le 07/07/2012 19:28, Davo Smith a écrit :
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module.
I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding. OK. I'll try that.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists.
The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed?
I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means).
"follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git).
Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there.
Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on.
Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed.
I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this. OK. That's correct.
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6824864
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5118 - Date: 08/07/2012
Jean FRUITET - St Sébastien / Loire
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6851103
Thanks you much Davo, This is the perfect "feuille de route".
I'll stick to it. Jean.
Le 09/07/2012 20:12, Davo Smith a écrit :
Yes - one commit per new feature / bug fix would be great - one commit per
line would be terrible!
I use the command line version of git and this is how I would proceed.
( git config --global color.ui auto // makes the output a lot prettier )
- git add remote davosmith git://github.com/davosmith/moodle-checklist.git // This adds my official repo as a source for your repo
- git fetch davosmith // Get all the latest code from my repo
- git checkout davosmith/master // Check out my master branch
- git checkout -b mynewbranch // Create a new branch, off my master branch, to put your code in (give it a better name than that!)
- Copy & paste in the code for the first feature / fix (try to avoid adding trailing whitespace or adjusting the layout of any of the existing code)
- git diff // to check exactly what you have changed - I can also recommend gitk (if you have it installed) to check
- git add FILENAME // for each file you have changed
- (optional) use gitk again, to double-check all the files / changes are queued up correctly; you can also use 'git status' to check the queued files as well
- git commit -m "Brief description of the feature that is being added" //
If it needs more explanation, leave out the -m "Brief description ...." and type into the text editor that appears a brief description, a blank line, then a more detailed explanation. 10. Go back to step 5 until all changes checked in 11. git push origin mynewbranch:mynewbranch // This pushes 'mynewbranch' to github and creates a new branch there called 'mynewbranch' (substitute the branch name you used in step 4) 12. Go onto github and put in a pull request from 'mynewbranch'
There are some alternatives to 'git add FILENAME' - you can also use 'git add *.php' (to add all PHP files in the current folder) or 'git add db' (to add all files in the db folder + sub folders) or 'git add .' (to add everything in the current folder and subfolders). You can also, skip the 'git add' altogether and call 'git commit -am "brief description"' if you know exactly what you have changed, as this adds and commits all the previously known files (but does not add any new files). Use this last option with great care (I have often committed changes that I didn't want,
or missed new files with this version).
One thing you might want to do BEFORE step 11, is to do another 'git fetch
davosmith' to see if I have changed anything (unlikely) and then a 'git rebase davosmith/master' to pull in my latest changes and then re-run your
changes over the top. Do not do this after you have pushed to github, as it won't work (without a --force parameter).
I hope that helps you out a bit with Git - feel free to ask a question if you get stuck ( [email protected] )
Davo
On Mon, Jul 9, 2012 at 5:07 PM, Jean FRUITET < [email protected]
wrote: Hi Davo,
I had never used GIT. Surely the way I did that fork is not the righ one.
I shall delete all that fork and redo a brand new one. I'll try to do a commit per new feature (but surely not one for each line of code !;>))
New features type 1: 1.1 - outcomes as list of items (affect import / export) 1.2 - outcomes validation made in any Moodle activity pushed to Checklist+ (affect cron job)
New features type 2: 2.1 - prove of practice attached to each item (affect table structure) 2.2 - Mahara connection (selected links / export data) (uses portfolio) Please feel free to give me your keen advice, for wich I'll thank you much.
Regards. Jean.
P.S.
I have presented the CheckList+ to French MoodleMoot 2012. http://moodlemoot2012.unimes.fr/ http://moodlemoot2012.unimes.fr/course/view.php?id=50
A lot of people interested by that evolution...
JF.
Le 07/07/2012 19:28, Davo Smith a écrit :
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module. I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding. OK. I'll try that.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists. The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed? I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means). "follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git). Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there. Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on. Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed. I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this. OK. That's correct.
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6824864
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5118 - Date: 08/07/2012
Jean FRUITET - St Sébastien / Loire
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6851103
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6854771
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5121 - Date: 09/07/2012
Jean FRUITET - St Sébastien / Loire
Hi Davo,
I have did the job for Feature 1 :
CheckList+ Feature 1 : Import / export outcomes files as Item list [jfruitet]
All the stuff is here : https://github.com/jfruitet/moodle-checklist/tree/checklistplus
It is a new branch called "checklistplus" based on a today fork of your https://github.com/davosmith/moodle-checklist
Many files modifed, of course...
Tomorrow I'll push the feature 2.
Regards,
Jean.
Le 09/07/2012 20:12, Davo Smith a écrit :
Yes - one commit per new feature / bug fix would be great - one commit per
line would be terrible!
I use the command line version of git and this is how I would proceed.
( git config --global color.ui auto // makes the output a lot prettier )
- git add remote davosmith git://github.com/davosmith/moodle-checklist.git // This adds my official repo as a source for your repo Correct command is git remote add davosmith git://github.com/davosmith/moodle-checklist.git
- git fetch davosmith // Get all the latest code from my repo
- git checkout davosmith/master // Check out my master branch
- git checkout -b mynewbranch // Create a new branch, off my master branch, to put your code in (give it a better name than that!)
- Copy & paste in the code for the first feature / fix (try to avoid adding trailing whitespace or adjusting the layout of any of the existing code)
- git diff // to check exactly what you have changed - I can also recommend gitk (if you have it installed) to check
- git add FILENAME // for each file you have changed
- (optional) use gitk again, to double-check all the files / changes are queued up correctly; you can also use 'git status' to check the queued files as well
- git commit -m "Brief description of the feature that is being added" //
If it needs more explanation, leave out the -m "Brief description ...." and type into the text editor that appears a brief description, a blank line, then a more detailed explanation. 10. Go back to step 5 until all changes checked in 11. git push origin mynewbranch:mynewbranch // This pushes 'mynewbranch' to github and creates a new branch there called 'mynewbranch' (substitute the branch name you used in step 4) 12. Go onto github and put in a pull request from 'mynewbranch'
There are some alternatives to 'git add FILENAME' - you can also use 'git add *.php' (to add all PHP files in the current folder) or 'git add db' (to add all files in the db folder + sub folders) or 'git add .' (to add everything in the current folder and subfolders). You can also, skip the 'git add' altogether and call 'git commit -am "brief description"' if you know exactly what you have changed, as this adds and commits all the previously known files (but does not add any new files). Use this last option with great care (I have often committed changes that I didn't want,
or missed new files with this version).
One thing you might want to do BEFORE step 11, is to do another 'git fetch
davosmith' to see if I have changed anything (unlikely) and then a 'git rebase davosmith/master' to pull in my latest changes and then re-run your
changes over the top. Do not do this after you have pushed to github, as it won't work (without a --force parameter).
I hope that helps you out a bit with Git - feel free to ask a question if you get stuck ( [email protected] )
Davo
On Mon, Jul 9, 2012 at 5:07 PM, Jean FRUITET < [email protected]
wrote: Hi Davo,
I had never used GIT. Surely the way I did that fork is not the righ one.
I shall delete all that fork and redo a brand new one. I'll try to do a commit per new feature (but surely not one for each line of code !;>))
New features type 1: 1.1 - outcomes as list of items (affect import / export) 1.2 - outcomes validation made in any Moodle activity pushed to Checklist+ (affect cron job)
New features type 2: 2.1 - prove of practice attached to each item (affect table structure) 2.2 - Mahara connection (selected links / export data) (uses portfolio) Please feel free to give me your keen advice, for wich I'll thank you much.
Regards. Jean.
P.S.
I have presented the CheckList+ to French MoodleMoot 2012. http://moodlemoot2012.unimes.fr/ http://moodlemoot2012.unimes.fr/course/view.php?id=50
A lot of people interested by that evolution...
JF.
Le 07/07/2012 19:28, Davo Smith a écrit :
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module. I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding. OK. I'll try that.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists. The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed? I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means). "follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git). Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there. Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on. Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed. I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this. OK. That's correct.
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6824864
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5118 - Date: 08/07/2012
Jean FRUITET - St Sébastien / Loire
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6851103
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6854771
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5121 - Date: 09/07/2012
Jean FRUITET - Université de Nantes - France
Thanks for that, I'll try to look at it as soon as possible, but I'm going to be on holiday for the next week, so will be a little while. On Jul 26, 2012 4:57 PM, "Jean FRUITET" < [email protected]> wrote:
Hi Davo,
I have did the job for Feature 1 :
CheckList+ Feature 1 : Import / export outcomes files as Item list [jfruitet]
All the stuff is here : https://github.com/jfruitet/moodle-checklist/tree/checklistplus
It is a new branch called "checklistplus" based on a today fork of your https://github.com/davosmith/moodle-checklist
Many files modifed, of course...
Tomorrow I'll push the feature 2.
Regards,
Jean.
Le 09/07/2012 20:12, Davo Smith a écrit :
Yes - one commit per new feature / bug fix would be great - one commit per
line would be terrible!
I use the command line version of git and this is how I would proceed.
( git config --global color.ui auto // makes the output a lot prettier )
- git add remote davosmith git://github.com/davosmith/moodle-checklist.git // This adds my official repo as a source for your repo Correct command is git remote add davosmith git://github.com/davosmith/moodle-checklist.git
- git fetch davosmith // Get all the latest code from my repo
- git checkout davosmith/master // Check out my master branch
- git checkout -b mynewbranch // Create a new branch, off my master branch, to put your code in (give it a better name than that!)
- Copy & paste in the code for the first feature / fix (try to avoid adding trailing whitespace or adjusting the layout of any of the existing code)
- git diff // to check exactly what you have changed - I can also recommend gitk (if you have it installed) to check
- git add FILENAME // for each file you have changed
- (optional) use gitk again, to double-check all the files / changes are queued up correctly; you can also use 'git status' to check the queued files as well
- git commit -m "Brief description of the feature that is being added" //
If it needs more explanation, leave out the -m "Brief description ...." and type into the text editor that appears a brief description, a blank line, then a more detailed explanation. 10. Go back to step 5 until all changes checked in 11. git push origin mynewbranch:mynewbranch // This pushes 'mynewbranch' to github and creates a new branch there called 'mynewbranch' (substitute the branch name you used in step 4) 12. Go onto github and put in a pull request from 'mynewbranch'
There are some alternatives to 'git add FILENAME' - you can also use 'git add *.php' (to add all PHP files in the current folder) or 'git add db' (to add all files in the db folder + sub folders) or 'git add .' (to add everything in the current folder and subfolders). You can also, skip the 'git add' altogether and call 'git commit -am "brief description"' if you know exactly what you have changed, as this adds and commits all the previously known files (but does not add any new files). Use this last option with great care (I have often committed changes that I didn't want,
or missed new files with this version).
One thing you might want to do BEFORE step 11, is to do another 'git fetch
davosmith' to see if I have changed anything (unlikely) and then a 'git rebase davosmith/master' to pull in my latest changes and then re-run your
changes over the top. Do not do this after you have pushed to github, as it won't work (without a --force parameter).
I hope that helps you out a bit with Git - feel free to ask a question if you get stuck ( [email protected] )
Davo
On Mon, Jul 9, 2012 at 5:07 PM, Jean FRUITET < [email protected]
wrote: Hi Davo,
I had never used GIT. Surely the way I did that fork is not the righ one.
I shall delete all that fork and redo a brand new one. I'll try to do a commit per new feature (but surely not one for each line of code !;>))
New features type 1: 1.1 - outcomes as list of items (affect import / export) 1.2 - outcomes validation made in any Moodle activity pushed to Checklist+ (affect cron job)
New features type 2: 2.1 - prove of practice attached to each item (affect table structure) 2.2 - Mahara connection (selected links / export data) (uses portfolio) Please feel free to give me your keen advice, for wich I'll thank you much.
Regards. Jean.
P.S.
I have presented the CheckList+ to French MoodleMoot 2012. http://moodlemoot2012.unimes.fr/ http://moodlemoot2012.unimes.fr/course/view.php?id=50
A lot of people interested by that evolution...
JF.
Le 07/07/2012 19:28, Davo Smith a écrit :
I am really sorry that it has taken me such a long time to get around to reviewing the changes you have made to the checklist module. I've started looking at the changes this afternoon, but I'm afraid that I'm struggling to find my way through the changes you have made. For me to have any chance of reviewing and integrating this, I'm going to have to ask you to create a cleaned-up version of this, with 1 commit for each new feature, along with a clear commit message explaining exactly what feature you are adding. OK. I'll try that.
Going through the commits, the first commit 'MODIF JF tags removed' adjusts the backup to include data tables that have not been added to the checklist module by that point. It also includes multiple restore_decode_rule statements, all for decoding the same string (CHECKLISTEDITBYCHECKLIST), but with different expected results. This is a clear bug, which you may have fixed in later commits, but it is difficult for me to locate where such a fix is, if it exists. The next two commits, both called 'Initial commit for JF fork' appear to be the same - are there some subtle differences I should be aware of? If not it would be helpful to include only one of these commits there. These also, unless I have misunderstood them, introduce more than one new feature / bug fix, so it would be much easier to review if they were multiple commits. You have partially documented your changes, but there are changes in files not mentioned in your README file. I notice one of the files has a large amount of commented-out SQL - is this needed? I'm not clear what the 'Correction bug course id' commit is doing.
The commit 'New branch :' has a non-descriptive commit message. It appears to be related to exporting to Mahara, but I would prefer to have a clear description of what it does and how it does it. It also includes at least one reference to 'mod-referentiel', a number of language strings which are not in alphabetical order (which makes ongoing maintenance much more fiddly) and a number of variables / comments / function names in French (I have nothing against the French language, but as I will need to maintain any integrated code in the future, it is much more helpful if I understand clearly what it all means). "follow_link" icon added as link ... - this looks fairly straight-forward, but I would prefer redundant code to be removed, rather than commented out (that is why we have version control software like git). Few bugs corrections about Mahara stuff ... - again, fairly straightforward. Maybe should be integrated back into your original Mahara commit, but not essential, as I can see what is going on there. Don't lock mahara stuff - this commit only seems to add a comment, that doesn't really explain what is going on. Signed-off-by: Jean FRUITET ... - looks like an obvious bug fix, but it would be helpful to explain exactly what is being fixed. I know that what I am asking here is probably a large amount of work. Unfortunately, from my perspective, reviewing the code in the way the commits are currently organised is also a large amount of work. Unless you are able to make my job a lot easier, I am afraid I will not be able to accept these changes. I am sorry if I am being unreasonable in anything I have written and I hope you understand why I am saying this. OK. That's correct.
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6824864
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5118 - Date: 08/07/2012
Jean FRUITET - St Sébastien / Loire
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6851103
Reply to this email directly or view it on GitHub:
https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-6854771
Aucun virus trouvé dans ce message. Analyse effectuée par AVG - www.avg.fr Version: 2012.0.2195 / Base de données virale: 2437/5121 - Date: 09/07/2012
Jean FRUITET - Université de Nantes - France
Reply to this email directly or view it on GitHub: https://github.com/davosmith/moodle-checklist/pull/3#issuecomment-7281765