|
|
Created:
12 years, 10 months ago by PhilEHolmes Modified:
12 years, 9 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionFix error messages in website build
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated pacth to get rid of website build warnings #Patch Set 3 : More changes to website build to make it quieter #
MessagesTotal messages: 19
I've at last uploaded a patch for review. This stops make website warning of missing files that we know are missing at this point.
Sign in to reply to this message.
good start! I'm not certain about the location of the text file (having it inside scripts/ seems a bit weird), but the framework is definitely there. http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... File scripts/build/extract_texi_filenames.py (right): http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... scripts/build/extract_texi_filenames.py:65: known_missing_files = '' could this be a list instead of a string? http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... scripts/build/extract_texi_filenames.py:87: known_missing_files_file = a known_missing_files.append(a) http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... scripts/build/extract_texi_filenames.py:94: missing_files = open (known_missing_files_file, 'r') missing_Files = open(...).readlines() then you don't need the next two lines. http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... scripts/build/extract_texi_filenames.py:121: if known_missing_files.find (include_name) == -1: if include_name in known_missing_files:
Sign in to reply to this message.
----- Original Message ----- From: <percival.music.ca@gmail.com> To: <PhilEHolmes@googlemail.com> Cc: <reply@codereview.appspotmail.com>; <lilypond-devel@gnu.org> Sent: Monday, May 02, 2011 4:20 PM Subject: Re: Fix error messages in website build (issue4428077) > good start! I'm not certain about the location of the text file (having > it inside scripts/ seems a bit weird), but the framework is definitely > there. Agreed. In the final analysis, I couldn't find a better place than to simply stick it next to the script file that uses it. If it was my source directory, I'd probably create a new directory off git or git/build and call it build_conf. > > http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... > File scripts/build/extract_texi_filenames.py (right): > > http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... > scripts/build/extract_texi_filenames.py:65: known_missing_files = '' > could this be a list instead of a string? I thought about whether a string was the _best_ solution, but decided that using a read() and find() was simplest and probably quickest. > http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... > scripts/build/extract_texi_filenames.py:87: known_missing_files_file = a > known_missing_files.append(a) Again, I'm assuming that this is changing from a simple string for the filename to a list? Strikes me we don't want lots of lists of missing files we can pass to the script - that'll just get confusing - I'd suggest just using a single file for any call. > http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... > scripts/build/extract_texi_filenames.py:94: missing_files = open > (known_missing_files_file, 'r') > missing_Files = open(...).readlines() > > then you don't need the next two lines. So with: string = open("filename").read() You don't need the close() ? > http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filen... > scripts/build/extract_texi_filenames.py:121: if known_missing_files.find > (include_name) == -1: > if include_name in known_missing_files: Depending on whether we do use lists... > http://codereview.appspot.com/4428077/ > > _______________________________________________ > lilypond-devel mailing list > lilypond-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/lilypond-devel > -- Phil Holmes
Sign in to reply to this message.
On Mon, May 02, 2011 at 05:29:27PM +0100, Phil Holmes wrote: > >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode65 > >scripts/build/extract_texi_filenames.py:65: known_missing_files = '' > >could this be a list instead of a string? > > I thought about whether a string was the _best_ solution, but > decided that using a read() and find() was simplest and probably > quickest. python has .readlines() for a file, and generally it's "pythonesque" to use lists. > >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode87 > >scripts/build/extract_texi_filenames.py:87: known_missing_files_file = a > >known_missing_files.append(a) > > Again, I'm assuming that this is changing from a simple string for > the filename to a list? Yes. wait... no -- we're still use a string for the filename, but using a list of strings for the names of files. um, let me get back to you on this. I've just confused myself. > Strikes me we don't want lots of lists of > missing files we can pass to the script - that'll just get confusing > - I'd suggest just using a single file for any call. It's not like that -- we still pass in a single filename. But the contents of that file are then read into a list variable, rather than leaving them in a string variable. > >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode94 > >scripts/build/extract_texi_filenames.py:94: missing_files = open > >(known_missing_files_file, 'r') > >missing_Files = open(...).readlines() > > > >then you don't need the next two lines. > > So with: > > string = open("filename").read() > > You don't need the close() ? That is not correct. With .readlines(), you don't need the close(). (at least, that's what I was told, but I don't see it in the docs. hmm, maybe I should change all my files!) oh wait, maybe it was a special case where you don't assign the file to a variable? Cheers, - Graham
Sign in to reply to this message.
----- Original Message ----- From: "Graham Percival" <graham@percival-music.ca> To: "Phil Holmes" <mail@philholmes.net> Cc: <PhilEHolmes@googlemail.com>; <percival.music.ca@gmail.com>; <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com> Sent: Monday, May 02, 2011 6:28 PM Subject: Re: Fix error messages in website build (issue4428077) > On Mon, May 02, 2011 at 05:29:27PM +0100, Phil Holmes wrote: >> >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode65 >> >scripts/build/extract_texi_filenames.py:65: known_missing_files = '' >> >could this be a list instead of a string? >> >> I thought about whether a string was the _best_ solution, but >> decided that using a read() and find() was simplest and probably >> quickest. > > python has .readlines() for a file, and generally it's > "pythonesque" to use lists. You'll be trying to convert me to Lisp next :-) >> >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode87 >> >scripts/build/extract_texi_filenames.py:87: known_missing_files_file = a >> >known_missing_files.append(a) >> >> Again, I'm assuming that this is changing from a simple string for >> the filename to a list? > > Yes. wait... no -- we're still use a string for the filename, but > using a list of strings for the names of files. > > > um, let me get back to you on this. I've just confused myself. No worries. You'd put that comment in the place where we extract the missing files list filename from the options. If we agree that there's only one list, I'll ignore the placement of the comment. >> Strikes me we don't want lots of lists of >> missing files we can pass to the script - that'll just get confusing >> - I'd suggest just using a single file for any call. > > It's not like that -- we still pass in a single filename. But the > contents of that file are then read into a list variable, rather > than leaving them in a string variable. OK. >> >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode94 >> >scripts/build/extract_texi_filenames.py:94: missing_files = open >> >(known_missing_files_file, 'r') >> >missing_Files = open(...).readlines() >> > >> >then you don't need the next two lines. >> >> So with: >> >> string = open("filename").read() >> >> You don't need the close() ? > > That is not correct. With .readlines(), you don't need the > close(). (at least, that's what I was told, but I don't see it in > the docs. hmm, maybe I should change all my files!) > > oh wait, maybe it was a special case where you don't assign the > file to a variable? My bet is that you don't really need file.close at all, since it's closed when the script exits (probably - although C# doesn't do this). I think it's best always to close explicitly. -- Phil Holmes
Sign in to reply to this message.
On 2011/05/02 17:43:12, email_philholmes.net wrote: > ----- Original Message ----- > From: "Graham Percival" <mailto:graham@percival-music.ca> > > python has .readlines() for a file, and generally it's > > "pythonesque" to use lists. > > You'll be trying to convert me to Lisp next :-) Have I mentioned yet that I've got the Structure and Interpretation of Computer Programs loaded on my kindle, ready for reading while I'm on holiday? :) Obviously I hope that I'll have nice scenery to look at on trains and stuff... but at the very least, when I'm going from London back to Glasgow (after my traveling companion has left for Vancouver), the first few hours are unlikely to be fascinating. > No worries. You'd put that comment in the place where we extract the > missing files list filename from the options. If we agree that there's only > one list, I'll ignore the placement of the comment. Ok, that sounds like a plausible mistake, and you've figured out what I meant to say, at least. Cheers, - Graham
Sign in to reply to this message.
----- Original Message ----- From: "Graham Percival" <graham@percival-music.ca> To: "Phil Holmes" <mail@philholmes.net> Cc: <PhilEHolmes@googlemail.com>; <percival.music.ca@gmail.com>; <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com> Sent: Monday, May 02, 2011 6:28 PM Subject: Re: Fix error messages in website build (issue4428077) > On Mon, May 02, 2011 at 05:29:27PM +0100, Phil Holmes wrote: >> >http://codereview.appspot.com/4428077/diff/1/scripts/build/extract_texi_filenames.py#newcode65 >> >scripts/build/extract_texi_filenames.py:65: known_missing_files = '' >> >could this be a list instead of a string? >> >> I thought about whether a string was the _best_ solution, but >> decided that using a read() and find() was simplest and probably >> quickest. > > python has .readlines() for a file, and generally it's > "pythonesque" to use lists. Just looked at this. readlines() leaves the newline character on the end of each string in the list and so therefore the filename isn't found in the list and therefore the error message is printed out. Now, it would be possible to append the \n to the filename before the test, or to loop the list and strip the \n, or a few other things, but it seems to me that this is adding code that is peripheral to the main aim of the script. My suggestion? Stick with the string version. -- Phil Holmes
Sign in to reply to this message.
On Tue, May 03, 2011 at 04:29:58PM +0100, Phil Holmes wrote: > ----- Original Message ----- From: "Graham Percival" > >python has .readlines() for a file, and generally it's > >"pythonesque" to use lists. > > Just looked at this. readlines() leaves the newline character on > the end of each string in the list and so therefore the filename > isn't found in the list and therefore the error message is printed > out. Yes, I've been bugged by this for a while. I finally did a google search for "python readlines newlines" and found this: http://stackoverflow.com/questions/544921/best-method-for-reading-newline-del... which suggests: lines = open(filename).read().splitlines() I'd rather use that method, and use lists for the names of files to ignore. Cheers, - Graham
Sign in to reply to this message.
I've got a new patch - do you want to look at it on your travels, or wait until you get back? -- Phil Holmes ----- Original Message ----- From: <percival.music.ca@gmail.com> To: <PhilEHolmes@googlemail.com>; <mail@philholmes.net>; <graham@percival-music.ca>; <email@philholmes.net> Cc: <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com> Sent: Monday, May 02, 2011 7:19 PM Subject: Re: Fix error messages in website build (issue4428077) > On 2011/05/02 17:43:12, email_philholmes.net wrote: >> ----- Original Message ----- >> From: "Graham Percival" <mailto:graham@percival-music.ca> >> > python has .readlines() for a file, and generally it's >> > "pythonesque" to use lists. > >> You'll be trying to convert me to Lisp next :-) > > Have I mentioned yet that I've got the Structure and Interpretation of > Computer Programs loaded on my kindle, ready for reading while I'm on > holiday? :) > > Obviously I hope that I'll have nice scenery to look at on trains and > stuff... but at the very least, when I'm going from London back to > Glasgow (after my traveling companion has left for Vancouver), the first > few hours are unlikely to be fascinating. > >> No worries. You'd put that comment in the place where we extract the >> missing files list filename from the options. If we agree that > there's only >> one list, I'll ignore the placement of the comment. > > Ok, that sounds like a plausible mistake, and you've figured out what I > meant to say, at least. > > Cheers, > - Graham > > http://codereview.appspot.com/4428077/ >
Sign in to reply to this message.
An update to use lists rather than strings.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
I think this just about completes this, unless we want to get rid of the 3 'Not a directory' messages.
Sign in to reply to this message.
woah, this is confusing. Could go back to the 2nd draft patch, yesterday's version? That is a very nice, self-contained patch. I'd like to have the official review period and get that pushed. I have no reason to believe that the other changes are bad; I just think that they're premature, and that we lose clarity by including them in this commit. You can^h^h^h^h^h^h^h^hOne can very easily get the previous draft, without losing the later work, in git. I'm not certain how to do it myself, though (I would begin by googling for tips), so I'm hesitant to offer to guide you through it. But it'll be a one- or two-line command, that a knowledgeable person can bash out in 30 seconds or so.
Sign in to reply to this message.
also, could we change the subject of the commit? (either by clever git stuff, or by starting a new commit on a separate git branch -- personally, I'd do the latter) Something like: Suppress known "missing" warnings in extract_texi (remember about the 50-char soft limit) The point behind the past two comments is that when you're looking at build system changes (or any changes, really), it's nice if you can see a nice short message that's really descriptive. When people look at the history, they're generally looking at dozens of commits. Having good subject messages (and a good amount of material in each commit) can make the changes a lot easier to understand.
Sign in to reply to this message.
On 5/6/11 7:03 PM, "percival.music.ca@gmail.com" <percival.music.ca@gmail.com> wrote: > woah, this is confusing. > > Could go back to the 2nd draft patch, yesterday's version? That is a > very nice, self-contained patch. I'd like to have the official review > period and get that pushed. > > I have no reason to believe that the other changes are bad; I just think > that they're premature, and that we lose clarity by including them in > this commit. > > You can^h^h^h^h^h^h^h^hOne can very easily get the previous draft, > without losing the later work, in git. I'm not certain how to do it > myself, though (I would begin by googling for tips), so I'm hesitant to > offer to guide you through it. But it'll be a one- or two-line command, > that a knowledgeable person can bash out in 30 seconds or so. You can use git stash (which I don't fully understand and have never used). But I'd recommend something like the following: git branch -f new-version git checkout new-version git checkout master git reset --hard <sha1ID of the old version> This will set your master back to the old version, and will create a new branch new-version that will be sitting there waiting for you to do further work on it. HTH, Carl
Sign in to reply to this message.
----- Original Message ----- From: <percival.music.ca@gmail.com> To: <PhilEHolmes@googlemail.com>; <mail@philholmes.net>; <graham@percival-music.ca> Cc: <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com> Sent: Saturday, May 07, 2011 2:03 AM Subject: Re: Fix error messages in website build (issue4428077) > woah, this is confusing. > > Could go back to the 2nd draft patch, yesterday's version? That is a > very nice, self-contained patch. I'd like to have the official review > period and get that pushed. > > I have no reason to believe that the other changes are bad; I just think > that they're premature, and that we lose clarity by including them in > this commit. > > You can^h^h^h^h^h^h^h^hOne can very easily get the previous draft, > without losing the later work, in git. I'm not certain how to do it > myself, though (I would begin by googling for tips), so I'm hesitant to > offer to guide you through it. But it'll be a one- or two-line command, > that a knowledgeable person can bash out in 30 seconds or so. > > > http://codereview.appspot.com/4428077/ OK. I'm not understanding how Rietveld works. On my machine I have 2 separate patches. The first is the one you responded LGTM to and I still have. My aim was then to do some more work to get rid of the rest of the warning, building on the previous patch. I did that, and fired it at Rietveld using git cl upload origin/master. Unexpectedly to me (since AFAICS it was brand new work) it appeared as patch 3 at http://codereview.appspot.com/4428077/. I expected it to be a new review. That said, if anyone wants to review my earlier effort, can't they just review Patch 2 at http://codereview.appspot.com/4428077/ ? If we are OK with the initial patch, I can email it to someone to push, and we could then review the new patch? -- Phil Holmes
Sign in to reply to this message.
On Sat, May 07, 2011 at 10:39:13AM +0100, Phil Holmes wrote: > OK. I'm not understanding how Rietveld works. On my machine I have > 2 separate patches. The first is the one you responded LGTM to and > I still have. My aim was then to do some more work to get rid of > the rest of the warning, building on the previous patch. I did > that, and fired it at Rietveld using git cl upload origin/master. > Unexpectedly to me (since AFAICS it was brand new work) it appeared > as patch 3 at http://codereview.appspot.com/4428077/. I expected it > to be a new review. ah, I see. To get it as a new issue, you'd have had to have done git cl issue 0 IIRC that isn't needed if the previous issue was closed, but you need that if it's still open. > That said, if anyone wants to review my earlier > effort, can't they just review Patch 2 at > http://codereview.appspot.com/4428077/ ? Yes, but as a general rule of thumb, since we have so few people willing to do any reviewing at all, I try to make it as easy as possible for them. > If we are OK with the initial patch, I can email it to someone to > push, and we could then review the new patch? well... ok. Send me that second-draft patch, and I'll push it. Then you can close that reitveld issue, and try another git cl upload to see if it'll start a new issue automatically (as long as the previous one is closed). Cheers, - Graham
Sign in to reply to this message.
Patchset 2 LGTM. A more complete explanation of why this is needed should be included in the commit message.
Sign in to reply to this message.
Phil Holmes wrote Saturday, May 07, 2011 10:39 AM > OK. I'm not understanding how Rietveld works. On my machine I > have 2 separate patches. The first is the one you responded LGTM > to and I still have. My aim was then to do some more work to get > rid of the rest of the warning, building on the previous patch. I > did that, and fired it at Rietveld using git cl upload > origin/master. Unexpectedly to me (since AFAICS it was brand new > work) it appeared as patch 3 at > http://codereview.appspot.com/4428077/. I expected it to be a new > review. The Reitfeld number is based on the git branch active when git-cl upload origin/master is called. All commits on that branch not in origin/master will be coalesced into a single patch and uploaded to Reitveld. If there has been a previous upload from that branch a new patchset is added to the previous ones. To generate a new Reitfeld issue you need to place the commits in a new git branch. > That said, if anyone wants to review my earlier effort, can't they > just review Patch 2 at http://codereview.appspot.com/4428077/ ? Yes, although general comments would need to be prefixed to indicate which patch set was being referenced. > If we are OK with the initial patch, I can email it to someone to > push, and we could then review the new patch? Sure - I could do that for you. Trevor
Sign in to reply to this message.
|