- m_conley (Mike Conley)
- Chipx86 (Christian Hammond)
- purple_cow (David Trowbridge)
- andrewhong_t (Andrew Hong)
- asalahli (Azad Salahli)
- dkus_ (David Kus)
- justy777 (Justin Maillet)
- mloyzer (Mark Loyzer)
- nicole_xin_ (Nicole Xin)
- rdone (Ryan Done)
m_conley (Mike Conley)
Hey all – so just took a look at the calendar.
we’re just a little over a month away from pencils down.
Wait; make that exactly 1 month away.
1 month – that’s 4 meetings remaining.
So take a look at where you are with your project.
4 weeks – and also keep in mind that other courses are likely to heat up;
wind of term, whatnot.
so you should be starting to think about orienting yourself to end of term – that means figuring out what you can accomplish in that time.
and more importantly, what you _cannot_ accomplish in that time.
and if what you cannot accomplish in that time is something you thought you _could_ accomplish in that time.
please let us know ASAP!
the sooner we know about corrections like that, the better.
The worst thing you can do is tell us that everything is fine, until the last week, and then tell us that you can’t deliver on your deadlines.
I think that’s all I have to open with.
Oh, one thing.
Additionally, we wanted to remind you all about the importance of the Description and Testing Done fields of your review requests.
It’s very, very important that these are clear – we use these to create the commit messages for your landings; the more comprehensive they are, the better.
So when you write them, don’t just target them assuming that we’re the ones reading them. Maybe it’ll be other developers, wayyyy in the future who are trying to piece things together.
So go into detail about what your review request does, and what sorts of things you tested.
That goes for currently existing review requests too – give them a read if you have any in flight, and if there are assumptions or other things unsaid that could be fleshed out, please do it.
Ok, I think that’s all
ChipX86: yes, just look at existing commits already made by us.
m_conley: Review Board has _excellent_ commit messages.
ChipX86: this is just as important as your code being in good shape.
and this, while it may not seem like a big deal, will really matter down the road.
m_conley: andrewhong_t: we’ll start with you.
andrewhong_t: sounds like you and ChipX86 are still figuring out some zip stuff?
andrewhong: Yes, I am going tor revisit some previous sites and documentation to see if i missed anything. I think we’re on the same understanding of things so hopefully it will be resolved shortly.
m_conley: andrewhong_t: ok, so “Anything regarding the zip feature that might help me would be great, or maybe an alternative approach that will also work?” <– that’s being sorted as we speak?
m_conley: andrewhong_t: excellent. And the autocomplete stuff – how’s that going?
andrewhong: most solutions I found on the web are about writing individual complete files to a zip but not chunks of files.
m_conley: andrewhong: tricky, yes
andrewhong: purple_cow put up a few reported issues about the autocomplete and I couldn’t reproduce 2 of them. I found the part thats affecting the last one
m_conley: andrewhong: ok
andrewhong: is there anything that you need from us?
ChipX86: andrewhong: I’m going to help with this after the meeting
andrewhong: a new router :p, otherwise nope
ChipX86, sounds good
m_conley: andrewhong: ok, thanks
rdone (Ryan Done)
m_conley: rdone: you’re next
m_conley: rdone: I wanted to talk diff slider first
m_conley: rdone: I was curious to know (and I should probably know this) if this diff slider is just for showing different versions individually
m_conley: or if it’s specifically for images, in that we do onion-skinning or transparency to check for differences?
rdone: as far as I understand yes
m_conley: rdone: ok
purple_cow: I think we want that to depend on whether the given review UI is capable of doing diffs
m_conley: I see, ok
purple_cow: if it is (I think ImageReviewUI is the only one that is right now), then we should allow selecting either a single revision or two.
If not, just a single revision
rdone: I’ll make a note of that
so if the media type is reviewable: display 2 versions
purple_cow: diffable, not reviewable
see the supports_diffing flag in ReviewUI
m_conley: rdone: so it sounds like you’re leaning into this slider now, and you’re blocked on reviews for the paginator.
rdone: is that about right?
I put most of my time this week into the paginator.
And I don
m_conley: rdone: ok cool, besides paginator reviews, is there anything you need from us?
any required clarity for the diff slider?
rdone: nope all good for now
I think I’m ok with the slider
m_conley: Great, ok. Thank you.
dkus_ (David Kus)
m_conley: dkus: you’re next
dkus: so it sounds like you had to do some refactoring on the backend…
m_conley: refactoring? Looks mostly like some cleanup
dkus: Lots of cleanup and some refactoring as well
m_conley: but ok, looks like you got a review go by
sorry, english. Looks like you’ve got another review request up that needs some looking at.
dkus: Yeah, so I addressed all the issues in my review request for the backend and its ready for more reviews now
m_conley: dkus: any questions about how to proceed on the front-end?
dkus: I think I’m good for now
m_conley: dkus: Ok. I know a previous student had been working on this stuff – did we judge that any of his front-end work was salvagable?
or are you starting from scratch here?
dkus: I’m starting with what the previous student did
I’ve already gone through it a bit and started cleaning some stuff up
m_conley: dkus: ok, that’s good. And you’re clear on how it operates? No questions on the pre-existing code?
dkus: None yet
m_conley: dkus: ok – let us know when you do. Thanks!
asalahli (Azad Salahli)
m_conley: asalahli: you’re next
asalahli: so I know for the last two meetings, there’s been some confusion about the scope and approach for rbt land.
m_conley: asalahli: are things clear now? Crystal clear?
asalahli: Yes it seems so.
I discussed it with ChipX86, and agreed upon a specific behaviour.
m_conley: asalahli: Ok, excellent. Do you think that the implementation is realistic within the 4 weeks remaining? That includes time for us to review the final patch.
asalahli: I’m almost certain it will be ready with Git support.
I cannot estimate time required for adding other SCM backends.
because I have no previous experience with them.
ChipX86: Git is a good start.
m_conley: asalahli: ok. Keep an eye on the clock – we lost some time figuring out this project, and then this week with other work, so you’ve gotta make sure you hit this one running
asalahli: m_conley, you’re right.
m_conley: asalahli: like, don’t panic or anything, but keep an eye on the calendar.
asalahli: anything blocking you, or anything you need from us?
I want to get a basic version that works by this week, so that we can focus on reviewing, correcting things during upcoming weeks, and I can focus on other SCM clients.
that’s my plan.
No, I don’t have anything blocking me for now.
m_conley: asalahli: ok, great – thank you!
mloyzer (Mark Loyzer)
m_conley: mloyzer_: you’re next
m_conley: mloyzer_: hey – sorry I forgot to ask you this earlier, but would you mind updating your review request with a document that you’ve generated so far?
just to see where you are?
mloyzer_: ya, sure.
m_conley: mloyzer_: great, thanks!
So, you ask: Do you have any tips on how to input the uploaded files or diff/change sets into the PDF? What do the Mimetypes do? Could I leverage that?
mloyzer_: when you say diff/change sets… are you talking about the actual patches?
was that in scope for this project? The actual export-to-foo of the entire patch?
mloyzer_: i was going to input the changes made to each file…
m_conley: so basically, the diff
ChipX86 / purple_cow: was that something we want to actually put into a PDF here? The full blown diff?
ChipX86: I don’t think so.
diffs are expensive and hard to do up-front
purple_cow: The only part of the diff I could see wanting are the lines corresponding to comments
m_conley: for file attachments, I doubt we could do much with anything except images
mloyzer_: so in the review summary, you just want the …summary of the review request change and not the code associated with it
m_conley: yes – there’s no need to export the diff as well
mloyzer_: oh ok.. so ..well and the comments/replies associated witht the reviews. correct?
m_conley: mloyzer_: yes, we do want those.
mloyzer_: ok, nevermind to my question then. I’ll make sure to post a PDF of what is currently generated today
m_conley: mloyzer_: great – is there any further clarity needed on what’s being exported here?
mloyzer_: I don’t think so
m_conley: mloyzer_: great, thanks!
nicole_xin_ (Nicole Xin)
m_conley: nicole_x-: hey!
nicole_x-: i’ve changed my mind about how to let user create a general comment
m_conley: nicole_x-: let’s hear it.
nicole_x-: in the status report i said to create body_top/general comment according to whether user tick “open an issue” or not
nicole_x-: that wouldn’t work since body top cannot have a header and a footer, and it’s confusing to the users.
so how about a scroll down menu on ‘Review’ button?
purple_cow: Can we just add an “Add Comment” next to the “Review” button?
nicole_x-: yes, and both body-top and general comments use a reviewDialogView?
ChipX86: comments always use a CommentDialogView
m_conley: purple_cow: –^?
andrewhong: m_conley, done. didn’t realize the seperate post in the google group
m_conley: andrewhong: thanks
purple_cow: the ReviewDialogView should show the general comments inside of it
just like it shows diff comments, file attachment comments, etc
body top and body bottom (header and footer) go on either side of the comments.
nicole_x-: okay, great!
I think i’m clear.
oh one thing!
m_conley: nicole_x-: go ahead
nicole_x-: ChipX86: i saw ur comments on my web-api resources, and you said some functions should be in subclass, not in baseGeneralCommentResource.
ChipX86: actually I have them separated, and purple_cow said there is no need to have a GeneralCommentResourse, so i just merge the subclass to the baseclass
ChipX86: can you give me more info on where the base class is used? What subclasses it?
there seems to be things in there i would not expect in a Base* class. Things reserved for specific resources.
nicole_x-: hmm, baseclass is BaseGeneralCommentResource.
subclass is GeneralCommentResource which doesn’t exist.
purple_cow: BaseGeneralCommentResource should be subclassed by ReviewGeneralCommentResource and ReviewReplyGeneralCommentResource.
nicole_x-: oh yes, i forgot to mention that
ChipX86: (pulling up the change)
ok, so for example, the name
can you rename that to BaseReviewGeneralCommentResource?
nicole_x-: yes sure
ChipX86: also, you don’t need to do the ‘general_comment_resource = BaseGeneral….()’ at the end, since it’s solely for subclasing.
nicole_x-: ok i will remove that!
m_conley: nicole_x-: any other questions, or anything blocking you?
ChipX86: but that makes more sense now. I wasn’t getting that it’s not used in the same way as the other base comment resources.
nicole_x-: yes it’s confusing
m_conley: i think i’m good now!
m_conley: nicole_x-: cool, thanks!
justy777 (Justin Maillet)
m_conley: justy777: ok, last but not least
m_conley: justy777: hey – so, you and I cleared up the webAPI testing questions you had before this meeting.
justy777: so are you all good now on that front?
justy777: yes, i am.
m_conley: justy777: ok, cool. So, how close are you to having all of this sandboxing wrapped up, would you say?
justy777: yes pretty close, just dealing with a few last things
m_conley: justy777: will you have the last patches up this week?
justy777: yes I will, and hopefully choose another project to work on
m_conley: justy777: please do, and please choose soon – and contact us if you can’t find anything.
justy777: is there anything else blocking you?
justy777: well, not really blocking but i always seem to be getting unbound method errors
ChipX86: in what cases?
m_conley: justy777: hrm – that’s the first I’ve heard of that. Curious as to why that wasn’t in your status report…
justy777: because I’ve solved most of them already and i just ran into more 5 minutes ago
purple_cow: it means you’re calling methods on classes instead of on instances
justy777: in this case it’s telling me TypeError: unbound method get_comment_thumbnail() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead).
ChipX86: is this with a spy_on call?
justy777: the spy_on call is not causing it, but there is a spy on that function yes
ChipX86: ok, is this new code since you last updated the review request?
justy777: it’s a new patch, so there is no review request yet, i can put up a [WIP] in a few seconds though.
m_conley: justy777: that’d be great – and please comment on the section of the code throwing this error.
justy777: is there anything else blocking you from moving forward?
justy777: okay i shall do that.
m_conley: ok, great