| OvenWerks | teward: yes that works. I like that it is much easier to read and easy to make sure there are spaces where they need to be without adding +" "+ kinds of junk. | 00:52 |
|---|---|---|
| OvenWerks | I have found another bug though :( I will look at fixing that. | 00:53 |
| teward | yep. this is why I like format strings :P | 01:05 |
| teward | OvenWerks: and what bug, if I may ask? | 01:05 |
| OvenWerks | the visible check box was not working right (just pushed fix) I could make something not visible, but could not make it visible again. And I had to do an edit to do even that. | 01:29 |
| OvenWerks | teward: ^^ So I have fixed that. (running glade to add one small change sure makes for a big diff) | 01:31 |
| teward | OvenWerks: I take it that was not my fault then :p | 01:34 |
| OvenWerks | no not your falt | 01:34 |
| Eickmeyer | OvenWerks: I just triggered a rebuild. | 01:37 |
| teward | Eickmeyer: might be a while :Po | 01:47 |
| teward | I can fast build if you want | 01:47 |
| Eickmeyer | teward: I'm in no rush. | 01:48 |
| Eickmeyer | Dunno about OvenWerks | 01:48 |
| teward | well this also lets me notice any odd new lintian warns :p | 01:49 |
| Eickmeyer | teward: Also, looks like it's already built and just needs to be publish. | 01:49 |
| Eickmeyer | teward: True. Go ahead. | 01:49 |
| teward | yep looks good. | 01:50 |
| teward | not sure if hte publisher will be fast enough :P | 01:50 |
| teward | but i'm impatient :) | 01:50 |
| teward | https://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v4.deb if you are as impatient as I :) | 01:51 |
| teward | glad to hear though my little change there didn't blow it up :) | 01:51 |
| teward | i thought for a second I had introduced another bug xD | 01:51 |
| Eickmeyer | Nah, looks like you're good. Any problems that came from that have been circumvented, I take it? | 01:53 |
| teward | Eickmeyer: AIUI yes | 01:54 |
| teward | OW said there was a bug in the visible checkbox not working right | 01:54 |
| teward | but not related to my code | 01:54 |
| teward | and I think OW is now in love with format strings xD | 01:54 |
| Eickmeyer | I see that. Looks like he fixed it. | 01:54 |
| Eickmeyer | hehehehe | 01:54 |
| teward | both f-strings | 01:54 |
| teward | and "".format :P | 01:54 |
| Eickmeyer | Maybe someday I'll learn Python and know what that means. | 01:55 |
| teward | heh | 01:56 |
| teward | Eickmeyer: basically? | 02:10 |
| Eickmeyer | Huh? | 02:10 |
| teward | instead of doing "some string "+variable+" was here" it's easier to know where things are in the string | 02:10 |
| teward | f"some string {variable} was here" <-- easier to make sure things are exactly where they need to be | 02:11 |
| teward | AND to quote out entire args | 02:11 |
| Eickmeyer | Okay, that makes sense. | 02:11 |
| Eickmeyer | I had to turn that into a bash script in my head. | 02:11 |
| teward | it's not actaully that far off ;) | 02:12 |
| OvenWerks | Eickmeyer: f-strings are like super bash instring variables as the "variables" can be equasions or the return of functions | 14:51 |
| teward | OvenWerks: can we confirm the bugs you ID'd are fixed and documented in the changelog and 'ready for upload'? | 16:17 |
| teward | and that 'this works'? | 16:17 |
| teward | once that's ready I can responsor it | 16:17 |
| OvenWerks | teward: they bugs are fixed... running my local file before uploading. I will DL the package and check this afternoon to check that way too. | 16:57 |
| OvenWerks | teward: in many ways I would like other people to try it as that exposes my blind spots... | 16:58 |
| OvenWerks | but we seems to have very few people willing to do that. | 16:59 |
| OvenWerks | When I build something, everything inside is "intuitive" to me... but I tend to be the odd one out in many of these things | 17:00 |
| teward | OvenWerks: well I don't run Studio | 17:12 |
| teward | so make Eickmeyer test it :p | 17:13 |
| * teward is just here as a 'helper' today | 17:13 | |
| * Eickmeyer tested it already | 17:25 | |
| Eickmeyer | teward: I'd go ahead. If we can get it into Eoan, I should have upload rights for bugfixes, or at least can get them from the DMB. | 17:26 |
| Eickmeyer | If we can get people testing the dailies, that would be the best route at this point. | 17:27 |
| * Eickmeyer is going crazy applying for jobs. | 17:27 | |
| teward | ack | 17:44 |
| teward | i will pull latest, debuild, and upload again. If it rejects I'll let you know. | 17:44 |
| Eickmeyer | teward: ack | 17:44 |
| teward | this is missing the subprocess patches... | 17:47 |
| teward | the one that switches the subprocess call to array based | 17:47 |
| teward | OvenWerks: did that get pushed? | 17:47 |
| teward | or did we expect me to apply that? | 17:47 |
| teward | or did that still not work? | 17:48 |
| teward | the change which does subprocess.run(["/usr/bin/exo-desktop-item-edit", str(self.tempfile)], shell=False) instead of the string based exec | 17:50 |
| teward | Eickmeyer: ^ this was the last bit that sarnold wanted done | 17:50 |
| teward | but it was untested | 17:50 |
| teward | 'cause i'm not on eoan | 17:50 |
| teward | nor STudio | 17:50 |
| teward | so if this still doesn't work i'll be happy to leave it as is with the strings | 17:51 |
| teward | but sarnold wanted it array-based preferably | 17:51 |
| teward | rather than string-driven | 17:51 |
| Eickmeyer | OvenWerks needs to address that then. | 17:51 |
| teward | ack | 18:11 |
| OvenWerks | I thought that had been changed. sorry, I will look at it. | 18:16 |
| OvenWerks | I thought that was what I tested. | 18:17 |
| OvenWerks | https://git.launchpad.net/ubuntustudio-menu-add/tree/usr/bin/ubuntustudio-menu-add?id=bd332ecd04cf778bd60208dbd187dd9658eaf7b4 | 18:19 |
| Eickmeyer | teward: ^ | 18:20 |
| OvenWerks | line 418 | 18:20 |
| teward | OvenWerks: that's the format-string driven one | 18:20 |
| teward | i had another patch hang on | 18:20 |
| teward | that applied it as an *array* with the full path | 18:20 |
| teward | guess it never pushed | 18:20 |
| teward | 'cause i didn't hit push | 18:20 |
| OvenWerks | I notice the terminal =true too :( | 18:21 |
| teward | yeah that's why it's not got my code in it. Mine uses shell=False | 18:21 |
| teward | and sarnold doesn't like shell=True if it can be avoided | 18:21 |
| teward | for security reasons | 18:21 |
| teward | okay... | 18:22 |
| teward | https://git.launchpad.net/ubuntustudio-menu-add/commit/?id=50791c079560173736ec8eaaa2dc4362b297c0e0 | 18:22 |
| teward | which uses the array | 18:22 |
| teward | lets see if *this* works. if not we'll revert | 18:22 |
| teward | this pulls the full path in so | 18:22 |
| teward | if this works as-is then all is good | 18:22 |
| teward | it SHOULD but it'll need a test ;) | 18:24 |
| OvenWerks | teward: that works here.... so what is different? When we tried it before it failed. Maybe shell == yes needs string? | 18:27 |
| teward | OvenWerks: no | 18:27 |
| teward | it's shell=False | 18:27 |
| teward | taht's what we needed | 18:27 |
| teward | which i discovered during some tests | 18:27 |
| OvenWerks | Yes, I agree | 18:27 |
| OvenWerks | I was wondering why it failed before (yesterday) | 18:28 |
| teward | yeah i was headscratching | 18:28 |
| teward | then i did some local Python subprocess call tests | 18:28 |
| teward | and found it | 18:28 |
| OvenWerks | The only differences I see are " rather than ', full path and Shell=false | 18:33 |
| OvenWerks | I think the shell=False could be omitted as being default but leaving it in is self documenting. | 18:34 |
| OvenWerks | teward: ^^ | 18:34 |
| teward | yeah I left it in there for explicitly making sure. I used double quotes by accident, but from Python's perspective 'foobar' == "foobar" as they're both strings | 18:35 |
| teward | if you prefer single quotes that's a minor change | 18:35 |
| OvenWerks | Not worth it | 18:35 |
| teward | mmkay. I need to add some changelog entries since I contributed now :| | 18:35 |
| OvenWerks | I tend toward double quotes... not sure why (K&R c maybe?) | 18:36 |
| teward | *SHRUGS* | 18:36 |
| teward | oops caps | 18:36 |
| teward | OK so I also added to the changelog since it has my two revisions per sarnold | 18:38 |
| teward | sarnold: want to do one last review before this gets reeval'd for sponsoring? | 18:38 |
| sarnold | sure | 18:39 |
| teward | https://git.launchpad.net/ubuntustudio-menu-add/tree/usr/bin/ubuntustudio-menu-add should be less offensive wrt subprocess and lib calls :) | 18:40 |
| sarnold | btw is there a reason why this line uses concatenation? | 18:43 |
| sarnold | self.tempfile = str(os.path.expanduser("~/"))+".config/ubuntustudio_menu_temp.desktop" | 18:43 |
| teward | sarnold: because OvenWerks hasn't gotten to using an f-string for it yet | 18:43 |
| sarnold | hmm I guess I could just try .. | 18:43 |
| teward | since they only learned about format strings yesterday-ish(?) | 18:43 |
| sarnold | os.path.expanduser("~/.does/not/exist") | 18:44 |
| sarnold | '/home/sarnold/.does/not/exist' | 18:44 |
| sarnold | no need for foramt strings | 18:44 |
| teward | sarnold: the concat is to add the rest. | 18:44 |
| teward | that's why the concat as is works | 18:44 |
| teward | and yes i'm annoying asking sarnold to spotcheck | 18:45 |
| teward | but since sarnold caught the first batch of errors | 18:45 |
| teward | i'm being cautious :) | 18:45 |
| teward | oh OvenWerks just a question | 18:46 |
| teward | self.menufolder in menu_cb | 18:46 |
| teward | that's not init'd in __init__ | 18:46 |
| teward | should it be? | 18:46 |
| teward | even if it's init'd to None? | 18:46 |
| teward | (code inspection shows it as a low-level consideration) | 18:46 |
| teward | (not blocking but still a question) | 18:46 |
| sarnold | teward: well, it looks like os.path.expanduser() doesn't need the file to exist, it should work fine without the concatenation or format string or hwatever | 18:48 |
| teward | indeed, it doesn't. | 18:48 |
| teward | sarnold: so you're saying straight os.path.expanduser("~/.config/ubuntustudio_menu_temp.desktop") ? | 18:48 |
| teward | (as an example) | 18:48 |
| sarnold | yeah that should work, right? | 18:49 |
| teward | yep even without the str-casting since it returns a str | 18:50 |
| sarnold | yay | 18:50 |
| teward | anything else glaring? | 18:51 |
| teward | Eickmeyer: OvenWerks: and forgive our bootprints over this :) | 18:51 |
| sarnold | nope, thanks for fixing up the executions to use arrays :) | 18:52 |
| teward | yeah that was a PITA to trace | 18:52 |
| teward | OvenWerks: at your convenience, `git pull` and test please :) | 18:53 |
| teward | if all still works | 18:53 |
| teward | i'll sponsor it up :) | 18:53 |
| Eickmeyer | teward, sarnold: I think I speak for both of us when I say any improvements are welcoe. | 18:57 |
| Eickmeyer | *welcome | 18:57 |
| teward | indeed. | 18:57 |
| teward | Eickmeyer: of course, sarnold does A LOT of code review so spots things I sometimes don't :) | 18:57 |
| Eickmeyer | Makes sense. | 18:57 |
| sarnold | everyone spots different things :) | 18:58 |
| sarnold | I'm good at noticing things along syscall boundaries | 18:58 |
| sarnold | and sometimes algorithmic mistakes | 18:58 |
| sarnold | other people know specific idioms in languages better | 18:58 |
| OvenWerks | teward: self.menufolder is actually initialized in the glade file. | 18:59 |
| sarnold | but now time to watch some women's world cup :) have fun | 18:59 |
| studiobot | <teward001> OvenWerks: ah. ack, OK. | 18:59 |
| studiobot | <teward001> just thought I'd ask :) | 18:59 |
| studiobot | <teward001> OvenWerks: if everything works fine here though as-is then this is Good 2 Go and I'll be happy to upload (unless you want a test build too) | 18:59 |
| OvenWerks | the data is static unlike submenu | 18:59 |
| OvenWerks | I am still testing, I looked at the init first | 19:00 |
| OvenWerks | I din't know I could give the whole string for expanduser. Thank you. Yes that still works. | 19:03 |
| OvenWerks | teward: thinking about logically, it does make sense I could do it that way. | 19:04 |
| * OvenWerks wonders how ubuntustudio-controls got through... | 19:05 | |
| Eickmeyer | OvenWerks: Probably because it was an already-existing binary. | 19:05 |
| OvenWerks | I will go through -controls sometime fixing some of these things there as well. | 19:07 |
| OvenWerks | The lines of code count should go down. | 19:08 |
| teward | OvenWerks: probably because it didn't have me or sarnold digging into the code :P | 19:08 |
| teward | (my coredev is new as of this week and Eickmeyer asked me to poke so :P) | 19:08 |
| OvenWerks | teward: I will at least go through the code first before we sak for a look. | 19:09 |
| studiobot | <teward001> 👍 | 19:10 |
| OvenWerks | I should be able to get rid of things like shell=True | 19:10 |
| teward | OvenWerks: yeah, since shell=True is already insecure | 19:10 |
| teward | they even say it in the docs xD | 19:10 |
| teward | thanks for the effort though, just make sure it works with the array execution and those strings being expanded | 19:10 |
| teward | if all looks good I'll kick it into NEW queue | 19:10 |
| OvenWerks | yup, but it is obvious to me my main problem there is using the shell to find the executable, just give the path and that can go away | 19:11 |
| OvenWerks | teward: ubuntustudio-menu-add looks good to me. anything those changes affect has been tested. | 19:11 |
| studiobot | <teward001> ack | 19:12 |
| studiobot | <teward001> are you and Eickmeyer comfortable with this being 0.1 in the repos then? | 19:12 |
| OvenWerks | yes | 19:12 |
| OvenWerks | when I work on -controls, I will commit each kind of change. (maybe tab to space frst) | 19:13 |
| studiobot | <teward001> good idea :) | 19:13 |
| * OvenWerks is off to lunch | 19:14 | |
| teward | OvenWerks: Eickmeyer: just as an FYI: the package was accepted to the NEW queue as is now | 20:49 |
| teward | it's waiting AA review | 20:49 |
| teward | at their convenience. | 20:49 |
| * teward goes to pack up before heading home from work | 20:50 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!