| *** rfolco_ has quit IRC | 01:10 | |
| *** dhellmann has quit IRC | 01:19 | |
| *** dhellmann has joined #openstack-sprint | 01:26 | |
| *** dhellmann has quit IRC | 01:31 | |
| *** dhellmann has joined #openstack-sprint | 01:31 | |
| *** dhellmann has quit IRC | 01:39 | |
| *** dhellmann has joined #openstack-sprint | 01:39 | |
| *** dhellmann has quit IRC | 01:45 | |
| *** dhellmann has joined #openstack-sprint | 01:46 | |
| *** dhellmann has quit IRC | 01:51 | |
| *** dhellmann has joined #openstack-sprint | 01:52 | |
| *** dhellmann has quit IRC | 01:59 | |
| *** dhellmann has joined #openstack-sprint | 02:00 | |
| *** rfolco_ has joined #openstack-sprint | 02:02 | |
| *** rfolco_ has quit IRC | 02:02 | |
| *** kien-ha has joined #openstack-sprint | 03:21 | |
| *** kien-ha has quit IRC | 03:23 | |
| *** baoli has quit IRC | 06:51 | |
| *** mrmartin has joined #openstack-sprint | 07:27 | |
| *** mrmartin has quit IRC | 08:05 | |
| *** pedroalvarez has quit IRC | 08:44 | |
| *** pedroalvarez has joined #openstack-sprint | 08:46 | |
| *** electrofelix has joined #openstack-sprint | 11:15 | |
| *** rfolco_ has joined #openstack-sprint | 11:40 | |
| *** waynr has joined #openstack-sprint | 12:50 | |
| *** zxiiro has joined #openstack-sprint | 13:12 | |
| electrofelix | waynr zxiiro zaro: looks like we have enough JJB cores to get started | 13:18 |
|---|---|---|
| electrofelix | zxiiro: kien-ha joining us? | 13:19 |
| *** baoli has joined #openstack-sprint | 13:19 | |
| zxiiro | electrofelix: yes he is | 13:20 |
| *** baoli_ has joined #openstack-sprint | 13:20 | |
| zxiiro | electrofelix: waynr so 2 patches i'd really like to land are the Views and Folder patches | 13:21 |
| *** kien-ha has joined #openstack-sprint | 13:21 | |
| electrofelix | zxiiro: I think they are probably best left after some of the refactoring, and probably try to focus on patches that either are closely related to the v2.0 api work or would be much easier to add now and move around a bit with the refactoring | 13:22 |
| zxiiro | sure | 13:23 |
| electrofelix | both the folders and views patches require work IIRC | 13:23 |
| electrofelix | I have an etherpad created - https://etherpad.openstack.org/p/jjb_api_v2.0 | 13:23 |
| electrofelix | if we can over the first two of these sessions, aim for just a small about of the initial patches of the V2.0 work, I think it'll start to become easier to review/rebase the remainder in a more async fashion | 13:24 |
| *** baoli has quit IRC | 13:25 | |
| zxiiro | looking now. do some of these need to be merged in order? | 13:27 |
| electrofelix | waynr: I've commented on all four patches at this stage (tried to do so ahead of time) | 13:27 |
| *** pedroalvarez has left #openstack-sprint | 13:28 | |
| electrofelix | to set a quick set of goals and expectations | 13:29 |
| electrofelix | not expecting these sessions to take 100% of peoples time | 13:29 |
| electrofelix | just that we make it a top priority versus other things you're busy with so as to expedite reviewing | 13:29 |
| electrofelix | Ideally we can help waynr out here by agreeing in principle whether the suggested review changes make sense and then pick a patch each to rework in line with the comments | 13:30 |
| electrofelix | ideally all 4 patches are merged by the end of today, and we have plans for the next set and align them | 13:31 |
| electrofelix | General familiarity with the V2.0 changes is more important | 13:31 |
| electrofelix | waynr: I can only think of one patch that I have that might be useful to land and rebase others on top: https://review.openstack.org/336090 -> it's a refactor of the test class inheritance, but you may also have done the same somewhere in your patch series | 13:32 |
| waynr | electrofelix: that sounds like a good plan, what i'm not sure of given the interdependent nature of this series of patches is if I really want to burden any one of us (myself in particular) with making fundamental changes to any one of them that require serious rebase efforts | 13:33 |
| electrofelix | I'm happy enough to help with the rebase efforts, it'll help me understand things if nothing else | 13:34 |
| waynr | in particular I am thinking of https://review.openstack.org/#/c/319611 where reverting API changes to the Builder class will have rebase consequences in later patches and it would be simpler to make the suggested change as a new patch | 13:35 |
| electrofelix | yeap, some of these might be easier done by adding a new patch rather than overhaulding the existing commit | 13:36 |
| electrofelix | if we can agree on what makes sense, some experimentation with rebasing/reworking can be done | 13:37 |
| *** cdelatte has quit IRC | 13:37 | |
| electrofelix | do we want to start with https://review.openstack.org/#/c/180252/? | 13:37 |
| *** cdelatte has joined #openstack-sprint | 13:37 | |
| waynr | sure, looking now | 13:38 |
| waynr | something to keep in mind about that the command line parsing change is that it begins in that patch and largely ends here: https://review.openstack.org/#/c/319617 | 13:40 |
| waynr | there are additional changes in the jenkins_jobs/cli/ directory in later patches but most of those have more to do with refactors going on throughout in other parts of the API | 13:42 |
| *** larainema has joined #openstack-sprint | 13:45 | |
| waynr | electrofelix, zxiiro so in one of my emails earlier this year I pointed out that some comments in earlier patches may already be addressed in later patches, I'm thinking it would be useful to adopt a convention for pointing out where this is the case | 13:45 |
| zxiiro | I think I'm fine with that if it's resolved in a later patch | 13:46 |
| waynr | okay, in response to comments where this is the case I will link to the patch where the issue is resolved | 13:47 |
| waynr | i'll add notes to the etherpad about these process issues just for future reference | 13:48 |
| waynr | okay, etherpad updated | 13:55 |
| electrofelix | thanks | 13:55 |
| *** mrmartin has joined #openstack-sprint | 13:55 | |
| waynr | whew, so many comments... | 13:56 |
| waynr | electrofelix++ | 13:56 |
| zxiiro | yeah good job | 14:00 |
| waynr | electrofelix: as for https://review.openstack.org/#/c/336090/ I don't think I've done anything quite along those lines, just from briefly looking over it I don't think it will conflict too seriously with anything I've done | 14:01 |
| waynr | most of my test changes focus on tests/cmd/ | 14:01 |
| electrofelix | that's good, and I'm happy to do some rebasing anyway | 14:01 |
| electrofelix | just found out precisely why the test inheritance change works https://review.openstack.org/336090 -> turns out our main problem was that importing a test class directly instead of the module results in it being added to the test suite | 14:02 |
| electrofelix | another good reason to import modules only | 14:02 |
| waynr | ah, interesting | 14:02 |
| electrofelix | no wonder I couldn't get that to work as expected before | 14:03 |
| *** mrmartin has quit IRC | 14:04 | |
| zxiiro | good to know | 14:04 |
| electrofelix | so one thing to watch out for, although it'll cause some rebase work, is that if someone needs to follow a line of code through the history of changes, the less number of patches that are involved in changing it the better. This may mean it makes sense to fix some style issues in some patches that introduce them rather than allowing them to be fixed later. Conversely, if it was in the same style before, then fixing it as a | 14:07 |
| electrofelix | I might add that as a rule of thumb for style based issue in helping decide when it makes sense to do the rework and when its better as a subsequent patch | 14:08 |
| electrofelix | going to get some coffee back shortly | 14:08 |
| waynr | I am getting a slow start on responding to feedback because I am still eating breakfast, if you can call chips and salsa with a side of sweet cherries "breakfast" | 14:10 |
| *** fergal_ has joined #openstack-sprint | 14:13 | |
| electrofelix | tasty breakfast? | 14:36 |
| waynr | yep | 14:36 |
| waynr | okay I am looking at the comments here: https://review.openstack.org/#/c/180252/10/jenkins_jobs/cli/entry.py | 14:37 |
| waynr | electrofelix: in your comment about consistency for add_argument help strings, would it be okay to be consistent in the other way so that they are always using triple single quote characters? | 14:38 |
| waynr | the benefit of triple quote characters is that it is less tedious to change the help text | 14:38 |
| electrofelix | waynr: I'd imagine it's more the indent styling that impacts changing the contents of that, over anything else | 14:40 |
| electrofelix | one thing I haven't check is whether multiline format impacts doc/man page generation | 14:40 |
| waynr | electrofelix: sure, indent changes are just as valid of a change as changing the content of the help text and in both cases the use of triple quote characters makes code change less of a chore | 14:42 |
| zxiiro | in my experience triple quote does affect some parsers if you also indent | 14:43 |
| zxiiro | so the safest way to use triple quote is to not indent on your subsequent lines | 14:44 |
| zxiiro | i can't recall which parses fail to ignore the starting whitespace | 14:44 |
| zxiiro | parsers* | 14:44 |
| electrofelix | zxiiro: when you say parsers, argparsers or document parsers generating content from an argparser instance? | 14:46 |
| electrofelix | going to see if I can dump the internal formatting, if it automatically tidies it up then I see no problem, it uses an internal multiline string to store it, I'd be far less comfortable that it's not going to cause issues in the future | 14:47 |
| zxiiro | electrofelix: yeah things that generate either t he CLI help or docs like sphinx | 14:47 |
| zxiiro | maybe the newer ones do a better job but I recall long ago I had something that didn't ignore the indents | 14:48 |
| zxiiro | need to step out for a meeting. let me know if i can help with some rebasing | 14:51 |
| electrofelix | waynr: looking at the internal representation of the parser object I see a number of helper strings in the following format | 14:52 |
| electrofelix | "Password or API token to use for authenticating towards\n Jenkins. This overrides the password specified in the configuration\n file.\n " | 14:52 |
| waynr | considering the preponderance of multiline strings in jenkins_jobs/modules/* used to generate documentation I would be surprised if this leads to a serious problem here, but jenkins_jobs/modules/* might work because they are class and function docstrings | 14:52 |
| electrofelix | waynr: I think that's exactly the difference, they get stored in a particular way | 14:52 |
| electrofelix | changing the format slight ensures it is stored as "help='Password or API token to use for authenticating towardsJenkins. This overrides the password specified in the configuration file.'" | 14:54 |
| electrofelix | I'll paste the format change I did for the help strings | 14:54 |
| electrofelix | into the etherpad | 14:54 |
| waynr | i though the issue was whether or not the generated documentation contains those newlines, not the internal representation | 14:55 |
| waynr | s/internal/intermediate/ | 14:55 |
| electrofelix | I was hoping that he internal representation removed the newlines when it was added, making it a moot point | 14:55 |
| waynr | I can see in the generated man page does not contain the newlines and you said that the sphinx documentation looks fine, is thee another type of generated documentation that we should be worried about? | 14:57 |
| electrofelix | I think it might anything that needs to use RawTextHelpFormatter, still looking, which means that if we need to add help and preserve newlines, we can't use triple quotes, it looks like argparse relies on the formatter to remove the newlines and whitespace, so if sphinx is using the same formatter it should be ok | 14:59 |
| waynr | wow, all of the JJB documentation is in the man page... | 15:00 |
| electrofelix | guess I'm wary where there might be surprises in the future and there exists an alternative styling that appears to achieve the same without any concerns about tweaking formatters | 15:00 |
| waynr | the alternative doesn't address my concern about making code changes a chore | 15:01 |
| waynr | is the concern that there might someday be a documentation system that can't handle the same legitimate python code that the argparse, sphinx, and manpage documentation generators seem to handle fine? | 15:05 |
| electrofelix | I think the problem is that some of us have experienced issues with it before, and it's unclear if it's completely gone away | 15:06 |
| electrofelix | having a quick look around | 15:07 |
| waynr | electrofelix: a good example of where this will be a pain in the ass with concatenated single-quotes is here: https://review.openstack.org/#/c/319609 where parser creation was refactored into a separate module | 15:19 |
| waynr | the indentation changes there as a result of convering an instance method to a function | 15:20 |
| *** fergal_ has quit IRC | 15:20 | |
| electrofelix | Seems it might be better to apply consistent style to all of these afterwards even though this is causing a lot of code churn in moving around | 15:22 |
| waynr | ...maybe not the best example though, considering that code was originally in jenkins_jobs/cmd.py, but it was impossible to have perfect knowledge of the final structure of this code at the beginning | 15:22 |
| zaro | sorry guys. i've been on vacation for last 2 weeks, will reappear on monday. | 15:25 |
| waynr | vacation++ | 15:25 |
| electrofelix | zaro: no worries, catch you at the session in two weeks ;-) | 15:25 |
| electrofelix | waynr: have you made any tweaks so far, I've found it's not too bad changing the format if you're ok with me aligning them all to the following format: | 15:34 |
| electrofelix | update.add_argument( | 15:34 |
| electrofelix | '--workers', | 15:34 |
| electrofelix | dest='n_workers', | 15:34 |
| electrofelix | type=int, | 15:34 |
| electrofelix | default=1, | 15:34 |
| electrofelix | help='number of workers to use, 0 for autodetection and 1 for ' | 15:34 |
| electrofelix | 'just one worker.' | 15:34 |
| electrofelix | ) | 15:34 |
| larainema | Hello, electrofelix waynr zxiiro zaro | 15:36 |
| zxiiro | alright i'm back | 15:38 |
| waynr | electrofelix: yeah i'm going through my second pass at reformatting this patch already | 15:42 |
| waynr | i'm more curious if this is an issue where you just prefer single quotes or have a concrete reason that we can use as a guideline to deny the use of valid python syntax not only in this case but in future cases as well | 15:43 |
| electrofelix | waynr: was going to suggest you skip changing the argparser stuff for now, and if you're happy with the suggested alternative in the etherpad I'll align the whole lot on that either at the end or by rebasing | 15:44 |
| electrofelix | I think it's more concern that I've seen problems with handling multi string text outside of docstring that often means you have to have zero indentation or use textwrap dedent to ensure consistency | 15:45 |
| waynr | for consistency's sake I am going through and reformatting all the argparse code for subcommands since you expressed a preference in an earlier comment on consistency with regard to either aligning the first argument to add_argument with the opening brace for the method or starting the arguments on the next line | 15:45 |
| waynr | when do we have to use zero indentation? | 15:46 |
| waynr | also, this is the first time you've mentioned textwrap.dedent | 15:47 |
| waynr | this sounds more like superstition than a rational reason to prefer one valid syntax over another | 15:48 |
| electrofelix | I've had to use it with python's logging class for nice formatting of the inline code and resulting output | 15:48 |
| electrofelix | argparse does appear to handle it through it's formatter class | 15:49 |
| waynr | well i am not going o undo the work i've done in the past hour to make the code formatting consistent, if you want to add a patch at the end of this series of patches to get it just the way you want it i will +2 it, i really just want to move on to API issues and get out of this style preferences rabbit hole | 15:51 |
| waynr | argh...english-- | 15:52 |
| electrofelix | ah, sorry I should have said earlier to leave the style piece until later, assumed you were going to follow your own guideline from the etherpad | 15:52 |
| waynr | i mean the work i've done in the past hour is to make the code formatting consistent, not that i don't want to make it consistent | 15:52 |
| zxiiro | Let's just merge it if there's no code issues and we can fix style in a later patch. I'm willing to help with rebasing if we have to | 15:53 |
| electrofelix | I think the primary concern about functionality was the use of '__' versus '_', apologies for letting the style drag you into the rabbit hole | 15:56 |
| electrofelix | waynr: from now on, style issues follow your initial rule of thumb, as long as you're happy with the proposed style change, we agree on who will sort that out and rebase the remaining patches afterwards so we can focus on behaviour | 15:57 |
| waynr | okay, for now with this first patch at least i am attempting at the moment to achieve a modicrum of consistency, typos leading to test failures though | 15:58 |
| waynr | also addressing the code issues | 15:59 |
| *** mrmartin has joined #openstack-sprint | 16:00 | |
| electrofelix | oh, one trick, not sure if you're aware of it, you can use the rebase command to execute tox against each patch in the series, but adding a exec entry after each entry and it'll stop at each one when there is a failure | 16:03 |
| waynr | i'll give that a shot | 16:04 |
| electrofelix | Just popped it into the etherpad | 16:06 |
| electrofelix | waynr: definitely to reduce the load on you, for anything after the first patch, I'm happy to tidy up the style issues, I have them all done for the argparser for the entire series | 16:10 |
| zxiiro | cool i didn't know tox lets you run that way | 16:15 |
| electrofelix | probably only worth doing it for the initial subset when targeting just the first set of patches | 16:16 |
| waynr | zxiiro: it's a git rebase capability, to run arbitrary commands between commits | 16:24 |
| electrofelix | waynr: regarding my comment about how the argparser options object is passed into the config object, modified and then returned, are you happy for me to change that by adding in a patch to the series after that one to overhaul do_magical_things? | 16:25 |
| waynr | electrofelix: i appreciate that offer re: style changes, it'll definitely make it easier for me to focus on API changes, bugs, and documentation which in my view should be the highest priority | 16:25 |
| waynr | electrofelix: i'm not sure which comment you are refering to | 16:26 |
| electrofelix | https://review.openstack.org/#/c/319609/2/jenkins_jobs/config.py line 171 | 16:26 |
| electrofelix | guess we're trying to work out the most efficient way of doing this | 16:27 |
| waynr | emacs-- ....i suddenly can't use magit's time machine mode for some inexplicable reason | 16:27 |
| electrofelix | maybe for the next session we try the following: | 16:29 |
| electrofelix | agree in principle the changes that make sense, then try to divide that out into items that people can work on independently to help, and then review again at the end to see if anything was missed | 16:29 |
| electrofelix | rather than starting on the first patch and trying to fix everything with that one before moving on | 16:29 |
| electrofelix | waynr zxiiro does that make more sense? | 16:30 |
| waynr | that sounds reasonable, might be difficult to subdivide the work in a way that doesn't lead to toe-stepping | 16:30 |
| waynr | if we can avoid style/formatting changes (put them in new patches at the end of the series) it might be less frustrating for me personally when we do step on each others' toes | 16:30 |
| zxiiro | I think it'd be better if we can figure out the dependency chain for them | 16:30 |
| zxiiro | like i'm sure not all the patches need tobe merged in order | 16:31 |
| zxiiro | but there's probably a few different flows | 16:31 |
| waynr | zxiiro: that's probably more true toward the end of the patch series | 16:31 |
| zxiiro | if we can break it down to a few buckets we can reassign the different buckets i think | 16:31 |
| electrofelix | maybe after the first 8-10 patches that's doable | 16:31 |
| zxiiro | yeah | 16:31 |
| electrofelix | looking around I think there is probably too much moving in those first ones | 16:32 |
| waynr | since i probably have the best familiarity with this work overall, i can try to identify a few different flows in preparation for our next sprint | 16:34 |
| electrofelix | also need a way of sharing some of the patches between ourselves if there is rework to be so we can show a suggestion without having to step on someones toes in submitting the patch series, it might not hurt for us to use a combination of github personal repos and gerrit to be able to directly share changes via multiple remotes? | 16:35 |
| *** mrmartin has quit IRC | 16:38 | |
| *** mrmartin has joined #openstack-sprint | 16:39 | |
| waynr | sure that sounds fine | 16:43 |
| *** mrmartin has quit IRC | 16:46 | |
| *** mrmartin has joined #openstack-sprint | 16:47 | |
| *** mrmartin has quit IRC | 16:52 | |
| waynr | whew, comments in first patch mostly addressed | 16:58 |
| waynr | and updates submitted | 16:59 |
| waynr | i have about 2 minutes to look at the second patch ;) | 16:59 |
| electrofelix | Looking through the first patch | 16:59 |
| zxiiro | great, I need to head out shortly too for lunch | 16:59 |
| electrofelix | I'm working on making my suggested changes for the second patch, and I'll put that up in a github repo for consumption | 16:59 |
| waynr | electrofelix: i also made some changes on that patch in this round of updates | 17:00 |
| waynr | i kind of started reading comments on both at the same time, also some merge conflicts had to be resolved during rebase | 17:00 |
| electrofelix | that's ok, I plan on re-syncing with your tree | 17:00 |
| *** rfolco has joined #openstack-sprint | 17:01 | |
| zxiiro | waynr: electrofelix I need to head out for lunch but I'll review the 1st patch when i get back | 17:01 |
| waynr | i'll probably try to address some more of the feedback on the other patches tonight, definitely before the next sprint | 17:01 |
| electrofelix | probably easier if I see a set of things that make sense if I can make those changes and share them with you and Thanh for each patch session rather than waiting until we meet up and then you're trying to make all the changes in one go | 17:02 |
| *** rfolco_ has quit IRC | 17:02 | |
| waynr | electrofelix: that sounds fair, would you want to just post a link to your github patches in the etherpad under the corresponding change link? | 17:02 |
| waynr | i'll leave the second patch alone until I see and have an opportunity to merge your changes | 17:03 |
| electrofelix | waynr: so one thing I've identified, is we definitely shouldn't use multiline strings for log messages or exceptions. The argparser seems to handle it cleanly via it's formatter (provided we never have a need to switch to the RawTextFormatter), but logging and exceptions will not remove the extra newlines and whitespace added by the mutliline string | 17:06 |
| electrofelix | so for argparse it appears to be a stylistic dogma as to which you use, logging and exceptions it does impact what is displayed | 17:07 |
| *** fergal_ has joined #openstack-sprint | 17:16 | |
| *** fergal_ has quit IRC | 17:16 | |
| zxiiro | what's happening? are we done for the day? | 17:39 |
| electrofelix | Mostly, have you reviewed the patch? | 17:43 |
| zxiiro | looking now, the 1st one right? | 17:43 |
| electrofelix | yeap | 17:43 |
| electrofelix | I've +2'ed it | 17:43 |
| electrofelix | and I'm working on some tweaks to go on top of the second one | 17:44 |
| zxiiro | Yeah I'm fine wit hthis patch. If there's issues we can fix it in subsequent patches. I think it's more important to get this moving at the moment. | 17:48 |
| electrofelix | So we call this session a wrap, we've landed one, but I think more importantly we've worked out a better way of progressing next time. | 18:09 |
| electrofelix | as well as answered some of the concerns around dealing with style tidy up | 18:09 |
| electrofelix | waynr: Here's the changes I was thinking about to untangle the config/parser objects so that the parser sets options in the config object instead of the config needing to know about the parser namespace at all | 18:27 |
| electrofelix | https://github.com/electrofelix/jenkins-job-builder/tree/untangle-config | 18:27 |
| electrofelix | have a bit of work to rebase fully the remaining patches, but thought it would be worth showing the idea | 18:27 |
| electrofelix | I'll finish rebasing the remaining patches on Monday and we can discuss then if you think the idea makes sense | 18:33 |
| *** electrofelix has quit IRC | 18:33 | |
| *** fergal_ has joined #openstack-sprint | 18:37 | |
| *** fergal_ has quit IRC | 19:01 | |
| *** fergal_ has joined #openstack-sprint | 19:47 | |
| *** baoli_ has quit IRC | 20:15 | |
| *** cdelatte has quit IRC | 20:26 | |
| *** fergal_ has quit IRC | 20:54 | |
| *** rfolco has quit IRC | 21:32 | |
| *** kien-ha has quit IRC | 21:54 | |
| *** rfolco has joined #openstack-sprint | 22:18 | |
| *** rfolco has quit IRC | 22:20 | |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!