<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:13px">Hi James,</span><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Congratulations on your first commit! Without wanting to nitpick too much there were a couple of things that I would like to comment on:</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Personally I would have preferred the function name as_miller_arrays_dict(), as this would make it a lot clearer (at least to me) what to expect from the function before looking at the documentation. At first I was confused because the 'map' part of the name made me think that it was doing something to do with electron density maps (especially given the context of miller arrays). Replacing 'map' with 'dict' in the name would avoid any potential for confusion.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Also, I would prefer the function arguments to be spelled out explicitly without using *args and **kwds, so that I can figure out what arguments it takes without having to read through the function source code to figure out that internally you are calling self.as_miller_arrays(), and then have to go and look up the function arguments for that. My IDE (WingIDE) shows me the function signature in its source assistant and also provides code completion for function arguments and keywords. However these won't be of much use when functions only list *args and **kwds.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Cheers,</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
Richard</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 17 April 2013 21:57, James Stroud <span dir="ltr"><<a href="mailto:xtald00d@gmail.com" target="_blank">xtald00d@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello All,<br>
<br>
Since this is my first commit, I just want to make sure that I have done these things in the appropriate way.<br>
<br>
Here's what I did:<br>
<br>
1. Added a wiki page: <a href="https://sourceforge.net/p/cctbx/wiki/Subversion%20set%20up/" target="_blank">https://sourceforge.net/p/cctbx/wiki/Subversion%20set%20up/</a><br>
2. Added the method as_miller_arrays_map() to iotbx_mtz_ext.object, which is declared in iotbx/mtz/__init__.py.<br>
3. Added a test for this functionality in the file iotbx/mtz/tst_miller_map.py<br>
<br>
Please let me know if the manner in which I did anything could use improvement.<br>
<br>
James<br>
<br>
_______________________________________________<br>
cctbxbb mailing list<br>
<a href="mailto:cctbxbb@phenix-online.org">cctbxbb@phenix-online.org</a><br>
<a href="http://phenix-online.org/mailman/listinfo/cctbxbb" target="_blank">http://phenix-online.org/mailman/listinfo/cctbxbb</a><br>
</blockquote></div><br></div>