Hi, This looks good. Maybe I will just apply your patches one by one as each looks reasonable. I can apply them on my system using syntax such as:
$ patch -p1 < 0001-Imported-the-expected-used-modules-in-lib.software.n.patch Before I apply them (which I cannot do until I return from holidays in the second week of July), I have a number of comments: patch 1 - No problems there, except for the missing '.' at the end of the "Progress sr #3043: ..." line in the commit message. patch 2 - The commit message should say rather than adding a docstring, that you added a stub of a function with docstring. The function that has no contents would be called a stub. The rest cannot go too wrong ;) patch 3 - For this one I have a few suggestions: 1) I would like to suggest an improvement to future proof this code - i.e. if the NMRPipe format slightly changes, then relax might still work. In this patch you assume fixed positions for the MODELINE and VARSLINE. But from the test_suite/shared_data/peak_lists/seriesTab.ser file, it looks like that these assumptions might be dangerous. For example the line with the 'Mode...' text is one of many REMARK lines. The line starting with the VARS text is after a number of REMARK lines. My worry is that it would be easy for nmrPipe to add more REMARK lines. Therefore I would suggest that we assume that there may be more than 14 header lines. I think this would be a fair assumption. Then most of the contents of this patch would be inside a loop over the file data. As you loop over the lines, you search for 'Mode:' and 'VARS', and then store the data when it is encountered. To find the number of header lines, have a counter for each line in the loop, then maybe if 'NULLSTRING' is searched for and then add 1 for the empty line (or search for the first data line starting with '1' and subtract 1) and use the 'break' statement to end the loop. Such an approach would be much more robust for future nmrPipe versions, and maybe also the current version if a user does something slightly differently. 2) All variables should be in lower case to match the relax coding style. Exceptions do exist, for example for parameters such as 'pA', but this is rare. 3) There are two empty lines at one point. 4) The third line of the commit message is unnecessary as all this information is already in the header line. patch 4 - It is best to avoid the re.match() function, and it appears to unused in all the patches anyway. The last line of the commit message could be merged into the header line. There's no need for 3 lines of commit messages for such a simple change. patch 5 - Again a number of points: 1) For Python 3, there should be spaces after all commas (at least the 2to3 Python 2 to 3 conversion program enforces this, so it must be important). 2) There are still references to Sparky present. 3) For the text "The peak intensity value " + repr(intensity) + " from the line " + repr(line) + " is invalid.", it is better to use the string formatting with %s. relax is slowly migrating to this format throughout, as it is faster, easier to read and easier for Python 3 (in some cases). 4) Again variables should be lower case (ASS_i and Z_A). 5) It is best to avoid highly Pythonic constructs such as enumerate(), filter, list construction using with mapping, zipping, for loops, etc. I.e. very Python specific ways of doing things. Avoiding this future-proofs the code. It is also easier on new relax developers who do not know Python. Keeping the constructs simple is a good thing. Nevertheless if the code is thoroughly checked using the test suite, a little bit of Pythonicness is ok. Cheers, Edward On 26 June 2013 15:25, Troels Emtekær Linnet <[email protected]> wrote: > Hi Edward. > > Just see this as a test round. :-) > > Most important is, that you can read these patch files. > In each patch, is also stored the commit message, which I > hope should be auto-read as well. > > I covered how it is possible to change the commit message. > I haven't tested yet, how it is possible to change the code commit. > > I guess that you rebase and amend commit. (Another option than "r") > git rebase -i HEAD~2 > > > The most problematic thing for me, was the understanding of the need to > create a branch from master. Add files. Create a new branch from the > just created branch. > Then change files. > Then do a "git format-patch PREVBRANCH" > > It's all about creating a lot of small branches, for each development step. > Merging, deleting them, comparing between them. > > Best > Troels Emtekær Linnet > > > 2013/6/26 Edward d'Auvergne <[email protected]>: >> Hi Troels, >> >> Would you be able to upload the patch either as the five original >> files or at least in a different format. On the Linux system I am >> using, I am currently unable to open RAR files. If you could use zip >> or tar.gz, that would be much better. On another note, why are there >> 5 patches and one commit message? The implementation of the function >> in lib.software.nmrpipe should only require one patch. Once I can >> read them, I'll start with the feedback. I may be able to do this >> tomorrow before I go on holidays next week. Also, I was wondering if >> you know how to use git to change the contents of a commit, as that >> should be possible? >> >> Cheers, >> >> Edward >> >> >> On 25 June 2013 19:06, Troels E. Linnet >> <[email protected]> wrote: >>> Follow-up Comment #33, sr #3043 (project relax): >>> >>> Completed NMRPipe SeriesTab reader for assignment according to SPARKY >>> format. >>> >>> Progress sr #3043: (https://gna.org/support/index.php?3043) Support for >>> NMRPipe seriesTab format *.ser >>> >>> Multiple spectra and intensity reading for NMRPipe SeriesTab formatted file >>> completed. >>> >>> ----------- >>> Zip file with 5 patches added >>> >>> Completed according to: >>> http://nmr-relax.kimlinnet.dk/index.php?title=Git_asynchronous_development#Complete_test_suite >>> >>> Patches should be applied with: >>> patch -p1 -i 0001-Imported-the-expected-used-modules-in-lib.software.n.patch >>> >>> patch -p1 -i 0002-Imported-the-expected-used-modules-in-lib.software.n.patch >>> >>> >>> >>> (file #18166) >>> _______________________________________________________ >>> >>> Additional Item Attachment: >>> >>> File name: seriestab_patches_v1.rar Size:4 KB >>> >>> >>> _______________________________________________________ >>> >>> Reply to this item at: >>> >>> <http://gna.org/support/?3043> >>> >>> _______________________________________________ >>> Message sent via/by Gna! >>> http://gna.org/ >>> >>> >>> _______________________________________________ >>> relax (http://www.nmr-relax.com) >>> >>> This is the relax-devel mailing list >>> [email protected] >>> >>> To unsubscribe from this list, get a password >>> reminder, or change your subscription options, >>> visit the list information page at >>> https://mail.gna.org/listinfo/relax-devel _______________________________________________ relax (http://www.nmr-relax.com) This is the relax-devel mailing list [email protected] To unsubscribe from this list, get a password reminder, or change your subscription options, visit the list information page at https://mail.gna.org/listinfo/relax-devel
