<div dir="ltr">Sorry Wolodja you're getting this email twice. When I clicked on reply it didn't send to pkg-clojure.<br><br>---------- Forwarded message ----------<br><div class="gmail_quote">From: <b class="gmail_sendername">Eugenio Cano-Manuel Mendoza</b> <span dir="ltr"><<a href="mailto:eugeniocanom@gmail.com">eugeniocanom@gmail.com</a>></span><br>
Date: Tue, Jul 16, 2013 at 12:43 PM<br>Subject: Re: Goals for next weeks<br>To: Wolodja Wentland <<a href="mailto:debian@babilen5.org">debian@babilen5.org</a>><br><br><br><div dir="ltr"><br><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
--------<br>
lein-xml<br>
--------<br>
* wonderful progress!<br>
<br>
* placeholder test/leiningen/test/core.clj<br>
<br>
write actual tests or remove this</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* check for file existence before writing to project.xml → warn user if it<br>
exists </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
* maybe extract the project-xml logic in the let clause into a standalone<br>
function<br></blockquote><div><br></div></div><div>Agree to all.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* fix indentation<br>
<br>
- in the reduce step, something like:<br>
(reduce<br>
(fn [deps d]<br>
(conj deps<br>
(xml/element :dependency {}<br>
(xml/element :name {} (str (d 0)))<br>
(xml/element :version {} (d 1)))))<br>
nil<br>
(:dependencies project))))]<br>
<br>
Which editor and indenting scheme do you use? I just use vim with the<br>
standard indenting rules (same as on emacs) and typically use indent by<br>
two spaces. As the code shifts massively to the left you might want to<br>
refactor the reduce step into a separate function.<br></blockquote><div><br></div></div><div>I use vim with 4 spaces. I'm also using the vimclojure plugin but you got me here, when I wrote this I didn't know how to indent it properly.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
lein_makepkg<br>
------------<br>
<br>
* Don't hardcode path separators. So code like this:<br>
<br>
outfile = 'debian/' + project['package_name'] + '.jlibs'<br>
<br>
should simply use os.path.join():<br>
<br>
os.path.join("debian", "foo.jlibs")<br></blockquote><div><br></div></div><div>Roger that</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* String construction with "foo" + "bar" is also a bit ugly and I find code along the lines of<br>
<br>
"{} {}".format("foo", "bar")<br>
<br>
much nicer. If you commonly fill these from a dictionary it makes sense to<br>
simply refer directly to the dictionary with, for example:<br>
<br>
"{foo} {bar}".format(**{"foo": 1, "bar": 2})<br>
<br>
See <a href="http://docs.python.org/2.7/tutorial/inputoutput.html#fancier-output-formatting" target="_blank">http://docs.python.org/2.7/tutorial/inputoutput.html#fancier-output-formatting</a> for details<br></blockquote>
<div><br></div></div><div>Will look it up but I personally don't like how it looks, maybe when I see it done I will change my mind</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* All the create_* functions are quite similar. It might make sense to abstract<br>
the functionality into a single function and simply call that.<br></blockquote><div><br></div></div><div>Agree</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
* No exception handling<br>
<br>
You don't handle any exceptions. It would be nice to fail gracefully if the<br>
program runs into exceptions, logs (cf. logging module) the failure and<br>
informs the user what is wrong.<br>
<br>
This holds true for file operations, but also for a lot of things that might<br>
throw IndexErrors tup[0] or so.<br>
<br>
What I mean here is that where you have:<br>
<br>
self._foo = parse(open(path))[0]<br>
<br>
it should rather be:<br>
<br>
try:<br>
with open(path) as in_file:<br>
foo = parse(parse)<br>
self._foo = foo[0]<br>
except IOError, ioe:<br>
_log.error("File {} couldn't be opened".format(path))<br>
_log.error(ioe)<br>
handle_error(...)<br>
except IndexError, indexe:<br>
_log.error(...)<br>
<br>
This makes the program much more robust and will also allow the user to find<br>
usage errors faster than just random tracebacks.<br></blockquote><div><br></div></div><div>True story. Sometimes I find myself trying to decrypt tracebacks. I planned to do this later but I guess it makes sense to do it now since the program is substantially larger.</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* logging<br>
<br>
This isn't really necessary right away, but the logging module *is* quite<br>
nice and you should accustom yourself with it.<br></blockquote><div><br></div></div><div>Will do.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
leinpkg/tools<br>
-------------<br>
<br>
* Don't hardcode paths and separators, but use os.path.join<br>
* Old-Style vs New-Style classes.<br>
<br>
Back in the bad old days (pre Python 2.2) built-in types (list, dict, ...)<br>
and user defined classes were very different beasts. I'd recommend to read<br>
<a href="http://www.python.org/download/releases/2.2.3/descrintro/" target="_blank">http://www.python.org/download/releases/2.2.3/descrintro/</a> and<br>
<a href="http://www.python.org/doc/newstyle/" target="_blank">http://www.python.org/doc/newstyle/</a> for an introduction to this topic. Since<br>
then a lot has changed, but one still has to derive new classes explicitly<br>
from object in order to get new style classes. In short, you shouldn't use:<br>
<br>
class Foo:<br>
<br>
but<br>
<br>
class Foo(object):<br>
<br>
* config_files -- no need to make this global<br></blockquote><div><br></div></div><div>Agree to all</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
PropertiesParser / PomParser / ProjectParser<br>
<br>
* old-style classes<br>
* class members/variables<br>
<br>
I don't quite understand why you initialise all those class variables and,<br>
in particular, items. If you *really* have to initialise them then just use<br>
instance variables (self.foo) and initialise them there.</blockquote><div><br></div></div><div>I did it to make the variables that the methods are expecting more explicit, but I don't think I understand what you're telling me. Could you show me an example?</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* get_items()<br>
<br>
I am not too fond of this design decision and you call this method exactly<br>
twice in lein_makepkg. These classes combined with this method are<br>
essentially just a wrapper around a dictionary that has been initialised<br>
with certain values. Unfortunately you do this is such a way that the actual<br>
classes no longer behave like a dictionary but are only a factory method for<br>
the magical "items" dictionary.<br>
<br>
What you *actually* want in the end is simply a dictionary or *some* data<br>
structure that contains the data you use to fill the templates. This data<br>
comes from different data sources and you rightfully make this explicit.<br></blockquote><div><br></div></div><div>The data structure that contains the data for the templates is the Project class. The goal of *Parser is to create a dictionary with some default values that can be used within the Project class. These values depend on whether they are being parsed from a POM, clj etc. Now that you mention it maybe I didn't choose the correct implementation for this particular idea but then how could I accomplish what I want? I do want these values (which I called properties) separated from the rest of the logic since there is really a set of properties inherent to the project we are packaging. Any ideas?</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
One thing you have to decide is how you want to access the data. There are<br>
two ways that you use at the moment:<br>
<br>
foo.bar (property/attribute)<br>
<br>
or<br>
<br>
foo['bar'] (dictionary key)</blockquote><div><br></div></div><div>Yes, I see a problem there. I'll think of something.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
You get data from multiple sources and essentially have to construct suitable<br>
dictionaries that you want to hand to jinja. These sources are:<br>
<br>
* global configuration dict (from configuration file in /etc)<br>
* user configuration dict (from conf file in ~/)<br>
* command line arguments<br>
* environment variables<br>
<br>
* project.clj<br>
* project.pom<br>
<br>
Please make each of these sources explicit and it is also of utmost<br>
important to have one place in which we model which setting can be set where<br>
and which setting can overwrite other. Lets discuss this later.<br></blockquote><div><br></div></div><div>Ok =) </div><div class="im"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
General Design - Data<br>
---------------------<br>
<br>
We have to think about the general design and should make it explicit what can<br>
be set where. Generally speaking what we want to do is:<br>
<br>
1. Gather data from multiple sources (see above)<br>
2. Combine/Merge data according to *explicit* rules<br>
3. Hand data to the templating engine<br>
<br>
What I would like you to do right now is to compile a table such as:<br>
<br>
TEMPLATE_ENTRY SOURCE (earlier overrides later)<br>
--------------------------------------------------------<br>
<br>
maintainer_email DEBEMAIL (env), EMAIL (env), .. (see dch)<br>
<br>
That should give us some idea on how to merge/construct the final<br>
dictionaries.<br></blockquote><div><br></div></div><div>I agree on the 3 steps that you mentioned, in fact the current implementation does that,</div><div><br></div><div>1. Gather data from multiple sources (see above): *Parsers (The OptionParser for options and the PropertiesParser {pom,project}parser for project properties.<br>
</div><div><br></div><div>2. Combine/Merge data according to *explicit* rules: The Project class where each rule is implemented with each _get_* method.<br></div><div><br></div><div>3. Hand data to the templating engine: The DebianTemplates.render_write method.</div>
<div><br></div><div>I understand what you're saying. If we want to be more explicit then maybe we have to create one single place where the merging is being done. With the current approach there's merging being done in OptionsParser and overrides in Project so if we want to change a rule maybe is more difficult. But if we want to easily add another source for properties or options how do we do it without modifying the rules that take care of creating variables for the templates? This sounds like the expression problem...</div>
<div><br></div><div>By the way here's the table (Based on what the Project class does):</div><div><br></div><div><div><div class="im"><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">TEMPLATE_ENTRY SOURCE (earlier overrides later)</span></div></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">------------------------------------------------------------</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">source_name prop(project/name) [1]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">package_name cmdline(-p), prop(project/name) [2]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">version prop(project/version), cmdline(-v) [3]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">description prop(project/description)</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">itp_bug cmdline(--itp), cmdline(--guess-itp) [4]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">homepage prop(project/url)</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">dependencies prop(project/dependencies) [5]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">genfiles cmdline(--jar), prop(project/name) [6]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">fullpath_deps prop(project/dependencies) [7]</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">use_javahelper cmdline(--javahelper)</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">maintainer_name cmdline(-m), env(DEBFULLNAME), conf(maintainer)</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">maintainer_email cmdline(-e), env(DEBEMAIL), conf(email)</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif">
<br></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[1] prop(project/name).replace('.', '-') + '-clojure'</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[2] cmdline(-p) _OR_ 'lib' + prop(project/name).replace('.', '-') + '-clojure'</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[3] re.search('\d[.\d]*', prop(project/version).group(0) _OR_ cmdline(-v)</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[4] cmdline(--itp) _OR_ if cmdline(--guess-itp): wnpp-check _OR_ 'XXXXXX'</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[5] [dep.split('/')[-1] for dep in prop(project/dependencies)] (PARSER)</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[6] [cmdline(--jar)] _OR_ [prop(project/name).replace('.', '-') + '.jar']</span></div>
<div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">[7] ['/usr/share/java/' + 'x' + '.jar' for x in [5]]</span></div>
</div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px"><br>
</span></div><div style="padding-right:1px;line-height:17px;font-size:13px;font-family:Arial,sans-serif"><span style="background-color:rgb(255,217,251);padding-top:1px;padding-bottom:1px">and titanpad link: </span><a href="http://titanpad.com/OGcNayAacG" style="font-family:arial;font-size:small;line-height:normal" target="_blank">http://titanpad.com/OGcNayAacG</a></div>
<div><br></div></div><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
One approach I typically like at least for command line arguments is to<br>
populate argparse's default values from os.environ (that way environment<br>
variables can be easily overridden by command line arguments).<br>
<br>
I guess that we have to make the merging of options a bit more explicit, but<br>
the important part is that you have *one* place in which you do that. You<br>
parse all available data individually and then combine them explicitly for<br>
each option.<br></blockquote><div><br></div></div><div>Ok so one place for everything.</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
PomParser / ProjectParser<br>
<br>
* remove items class variable<br>
* Don't parse in __init__ (or rather don't initiate the parsing here)<br>
but write an explicit method for the parsing which returns the resulting<br>
dictionary.<br>
<br>
I don't quite like the design of these and would prefer one of two approaches:<br>
<br>
1. Return a (conceptionally static) instance that simply populates attributes<br>
from a POM file.<br>
<br>
The approach here would be something like:<br>
<br>
class POMParser(object):<br>
<br>
def __init__(self):<br>
self._xpaths = {<br>
'name': "/pom:project/pom:artifactId/text()",<br>
'version': "/pom:project/pom:version/text()",<br>
...<br>
}<br>
self._ns = {'pom': '<a href="http://maven.apache.org/POM/4.0.0" target="_blank">http://maven.apache.org/POM/4.0.0</a>'}<br>
<br>
<a href="http://self.name" target="_blank">self.name</a> = None<br>
self....<br>
<br>
def parse(self, path):<br>
"""Parse given ...<br>
<br>
"""<br>
try:<br>
with open(path) as in_file:<br>
self._tree = et.parse(in_file)<br>
except IOError, ioe:<br>
log_error(...)<br>
handle_error(...)<br>
<br>
@property<br>
def name(self):<br>
return self._tree.xpath(self._xpaths['name'}, namespaces=self._ns)<br>
...<br></blockquote></div></div><div>ARRGGG why didn't I think of self._xpaths = {} !!?? much cleaner than what I have.</div><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
*OR*<br>
<br>
2. Write an explicit parser method that returns a dictionary<br>
<br>
class POMParser(object):<br>
<br>
def __init__(self):<br>
self._xpaths = {<br>
'name': "/pom:project/pom:artifactId/text()",<br>
'version': "/pom:project/pom:version/text()",<br>
...<br>
}<br>
<br>
self._ns = {'pom': '<a href="http://maven.apache.org/POM/4.0.0" target="_blank">http://maven.apache.org/POM/4.0.0</a>'}<br>
<br>
def _name(self):<br>
return self._tree.xpath(self._xpaths['name'}, namespaces=self._ns)<br>
<br>
def parse(self, path):<br>
"""Parse given ...<br>
<br>
"""<br>
try:<br>
with open(path) as in_file:<br>
self._tree = et.parse(in_file)<br>
except IOError, ioe:<br>
log_error(...)<br>
handle_error(...)<br>
<br>
return {<br>
"name": self._name()<br>
"foo: self._foo()<br>
}<br>
<br>
Or something along those lines. Comments from Paul/Algernon?<br></blockquote><div><br></div></div><div>Hmm I like them both. They are better than the mixed thing we currently have.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Project class<br>
-------------<br>
<br>
* don't set class variables<br>
<br>
We don't really need this "I contain all the data class" -- Just construct<br>
dictionaries you hand to jinja explicitly according to the rules.<br>
<br>
We should discuss the general design and nail it now.<br></blockquote><div><br></div></div><div>As stated above.</div><div><br></div><div>Wolodja thank you so much for your feedback. I'm very happy with the comments and I find very positive that we have a lot of room for improvements. I'll be working on the more obvious ones while we get more feedback from paultag and algernon on the critical ones. <br>
<br>Cheers!<span class="HOEnZb"><font color="#888888"><br>Eugenio</font></span></div></div></div></div>
</div><br></div>