Meeting Minutes: November 16, 2014

Attendees

Mentors

  • m_conley (Mike Conley)
  • ChipX86 (Christian Hammond)
  • purple_cow (David Trowbridge)

Students

  • brennie (Barret Rennie)
  • dkus (David Kus)
  • nicole_xin (Nicole Xin)
  • andrewhong (Andrew Hong)
  • rdone (Ryan Done)
  • asalahli (Azad Salahli)
  • justy777 (Justin Maillet)

Announcements

We’ve only got a few weeks left in the term. Two more meetings and then it’s pencils down. So your projects should be approaching finish – you should have patches up and out of WIP before pencils down with enough time for a few review iterations.

Round Robin

brennie

  • m_conley: So you’re working on multiple commits per review request. Do you have a plan to move forward with, then?
  • brennie: Yeah. I just need to work out how the DB should be changed to reflect multiple diffs attached to a RR.
  • brennie: (the hackpad needs to be cleaned up, its mostly just a dump from IRC)
  • ChipX86: We know there’s only so much that can be done this semester.
  • m_conley: What’s the scope of this, considering the remaining time? Like, how much do you aim to get done before pencils down?
  • brennie: I want to add an extra API endpoint that will be able to handle taking multiple diffs and attaching them to a RR.
  • brennie: Modifying the DB to allow multiple commits per RR, and allowing RBtools to talk to that endpoint.
  • brennie: I don’t plan to start on the frontend until thats all done.
  • m_conley: Is there anything we can provide you? Anything that you need from us? I know you’re waiting on a review…
  • ChipX86: I plan to do a few rounds of reviews tonight. Been pretty unavailable the past few days
  • brennie: Where is the RR model? There is no reviewboard/reviews/models.py
  • ChipX86: It’s in reviewboard/reviews/models/
  • purple_cow: There’s a models/ directory
  • m_conley: models.py got too big so it got split up a while back

dkus

  • dkus: Not many questions from me, I still need reviews on my backend work.
  • m_conley: I took a look at your front-end patch too. Wouldn’t mind seeing some screenshots posted: https://reviews.reviewboard.org/r/6510/
  • dkus: Yeah I’ll post ss of what I have.
  • dkus: And I’ll need some input soon (maybe from ChipX86) on deciding how those drop overlays should look.
  • ChipX86: Ok.
  • m_conley: I’m guessing we might want to echo what we do for attachment drag/drop upload for review requests, but yeah, let’s iron that out when we get there.
  • m_conley: Anything else blocking you, or anything you need from us?
  • dkus: Nope, I’m a little concerned about finishing this all in the next 2 weeks though.
  • m_conley: Ah, I see. What do you see is the biggest hurdle to get through?
  • dkus: Well, once I have the drop overlays all done and tested, I still need to look at showing progress on uploading files (unless we don’t need that).
  • ChipX86: I wouldn’t make that a priority.
  • m_conley: Considering the remaining time, along with time for review, yea: we might want to do that in a follow-up.
  • dkus: Alright. It’s something I wouldn’t mind doing after the semester if I don’t have time for it before the end.
  • m_conley: That’s music to our ears.
  • m_conley: Ok, so if we chop off the progress feedback, does that make getting something landable finished more feasible?
  • dkus: Yeah.
  • m_conley: Excellent.

nicole_xin

  • nicole_xin: I just resolved reviewDialogViewTest this morning.
  • m_conley: That’s good. How about the second issue in your report? “One or more fields had errors”?
  • nicole_xin: So, about general comment reply, I was wondering any hints on that 400 Bad Request error?
  • nicole_xin: Still blocked by this one
  • ChipX86: What does the error payload say?
  • nicole_xin: I got this:

    {“fields”: {“include_text_types”: [“Field is not supported”]}, “stat”: “fail”, “err”: {“msg”: “One or more fields had errors”, “code”: 105}}

  • ChipX86: We made some big changes to how markdown support works in master. You’ll need to update some stuff in your API resource to match it.nicole_xin: But I check the request, everything seems normal, and include_text_types contains: raw
  • ChipX86: I’ll take a look at your change and try to point you in the right direction there.
  • nicole_xin: Alright thanks ChipX86!
  • m_conley: ChipX86: is there an example for nicole_xin to work off of? A diff of another resource that needed similar changing?
  • ChipX86: Well, there’s a series of changes.
  • ChipX86: Let’s dig into this after the meeting.
  • nicole_xin: Sounds good.
  • nicole_xin: I think general comments can be finished next week. I’m thinking about picking another short project.
  • m_conley: It’ll have to be pretty short
  • nicole_xin: Probably ‘rbt stamp’?
  • ChipX86: That might be more than a week or two’s worth of work, but you could make some progress toward it.
  • m_conley: Let’s give it a shot.
  • nicole_xin: Great. Oh I need some clarification here.
  • nicole_xin: rbt stamp just need to stamp the current commit with a review_request url?
  • ChipX86: Yes, but that requires making use of the same guessing ability that rbt post -u uses, which means moving that out into a utility function (I don’t think we landed a change for that yet?).
  • m_conley: A little light refactoring.
  • ChipX86: So that + optionally taking an ID (using rbt stamp -r ), and then augmenting the commit.
  • ChipX86: I think that part may require a bit more work. We have support for editing a new commit, but not augmenting one yet. Shouldn’t be too bad, if you just focus on git for now.
  • nicole_xin: I don’t have further questions.

andrewhong

  • m_conley: So, glad to see all the testing you’re doing on r/6333. Thanks for filling that data in. I’m assuming you’re just waiting for review on that patch now?
  • andrewhong_p: Yeah, but I found out that the matchContains flag doesn’t actually work so I’m looking into that, and then have to retest the change to make sure its implemented properly.
  • m_conley: Alright. Can you ping one of us when you’ve got that figured out so we can get that fix re-landed?
  • andrewhong_p: Sure!
  • m_conley: So, attachment ZIP downloading… Is that kinda mothballed while you deal with this autocomplete thing? I don’t see much about it in your status report.
  • andrewhong_p: I realize I probably wont finish by the end of the term but I am willing to finish it up after the term.
  • m_conley: What is remaining with the attachment zip stuff?
  • andrewhong_p: Implementing chunking, basically integrating ChipX86’s code snippet. I decided to keep focus on autocomplete this week.
  • m_conley: So can you give us a sense of what you think you’re going to have done in time for review before pencils down?
  • m_conley: What do you think you can realistically finish?
  • andrewhong_p: I’m hoping I can wrap it up entirely by the end of term.
  • m_conley: What are we talking about here? Autocomplete? Attachment downloading?
  • andrewhong_p: Both
  • m_conley: Wow, ok. Yes, please try to lean on it, push it through. We think you can do it with a good hard push. Just let us know when you’re blocked, and we’ll unblock you ASAP
  • m_conley: And you should have enough time to get these both ready to roll out. Sound good?
  • andrewhong_p: Sure thing, thanks. I should have lots more time after Wednesday this week.

rdone

  • m_conley: So sounds like you got unblocked, which is good
  • m_conley: Do you feel confident that you can get your project done by pencils down with time for review?
  • rdone_: I think I should get a significant chunk done: stuff working well, but maybe not 100% with tests and making sure it works flexibly with different file attachments
  • m_conley: What file attachments are you planning on having it work with?
  • rdone_: I think just image and text for now, but the fileAttachmentRevisionSlider could be generically used by any fileAttachment
  • ChipX86: Those are the only kinds we natively support in RB anyway. If they work for both, we’re in good shape
  • m_conley: Some documentation on how future developers (or extension writers) can support the slider for other attachments would be good
  • rdone_: Yeah i think that’s easier than trying to predictively test stuff
  • m_conley: Perhaps those should be your final deliverables – tested and working for images and text, and documentation for how to extend further. Does that sound realistic?
  • rdone_: Yeah for now
  • m_conley: Let us know if/when that changes ASAP, ok?
  • rdone_: For sure

asalahli

  • asalahli: justy777: Sorry for delaying you 😦
  • m_conley: So the question you’re referring to in your status report – that’s regarding error handling?
  • asalahli: Yes. So, as heapify mentioned, one option is to pass ignore_errors=False and check return code, but merging and pushing uses 4-5 commands and checking the return code of every of them will complicate the code. And as I mentioned above, with that approach I cannot handle the errors in command main function, so thats why I added optional exception. And in the future with more scm support, I’m guessing it might scale beter
  • m_conley: In my experience, it’s strange for a function parameter to change the exception-throwing behaviour of that function.
  • ChipX86: Yeah, we should really be checking the return code instead of causing exceptions to occur. Exceptions would be fine if that was the only way of reporting errors in execute(), but it’s not.
  • m_conley: Do you feel like you can get this wrapped with time for review before pencils down?
  • asalahli: Actually, I think it is ready for full blown reviews by now (I’m hoping I haven’t missed any big detail in the implementation).
  • ChipX86: I’ll give it a review tonight
  • asalahli: The only thing I’d change is error handling. Oh, speaking of which: do we actually need special error handling in case of execute()? If execute fails, it prints the output and terminates the command. So, I’m asking would it be better to just stick to that behaviour?
  • ChipX86: What I’d suggest though is catching the errors and having a reasonable result from the function, so that we can standardize the results from any SCMClient.whatever() function
  • asalahli: I see. What should SCMClient.whatever() functions return when error occurs? error message itself? or just true false?
  • ChipX86: It can raise an exception, like a MergeError with a description
  • asalahli: Ok. One more question: should I define two seperate exceptions for merging and pushing to upstream, or a common exception is fine?
  • ChipX86: I’d say have a MergeError and a PushError

justy777

  • m_conley: So, you’ve got some hooks written – that’s great! With two remaining weeks, accounting for time for review, how many more hooks do you think you can polish off including the ones you’ve already put up?
  • justy777: There are 4, I would guess two this week could be polished off maybe 3 if im lucky. the 4th is up in the air.
  • m_conley: Wait – to be clear, those 4 include the ones you’ve already got up? Or are separate from the ones you’ve already put up?
  • justy777: Yes they do, 3 [WIP]s need to be polished, one of those that is up is iffy because of the way SCMTools are stored
  • m_conley: Ok – you said the 4th is iffy. Why is it iffy?
  • justy777: Because ChipX86 told me that SCMTools are stored in the database and the hook as of them could not be cleanly implemnted, and that I should work on the others first.
  • ChipX86: Yeah, I wouldn’t worry about the SCMTools. That’s going to be a much bigger, substantial project.
  • m_conley: Let’s get these three cleared out then, and some solid documentation.
  • ChipX86: Documentation-wise, I’d really like to see new docs for the hooks in the manual, like we have for all other hooks, complete with working examples
  • m_conley: Examples are the best – often those are what the eyes latch onto immediately
  • m_conley: Do you have any questions on any of that?
  • justy777: Just one. There are not a lot of tests for simple hooks like two of the ones I’ve been working on, should I be testing the register and unresgister work indivdually or together and should they be more thououghly tested?
  • ChipX86: Things should always be tested individually
  • purple_cow: Just because existing ones aren’t thoroughly tested doesn’t mean new ones shouldn’t be
  • m_conley: Yeah, let’s go for finer grain testing
  • ChipX86: When writing unit tests, I try to write a test for all generic use cases for that code, and try to think if there are edge cases that make sense to test
  • purple_cow: And we can blame the older ones on the people who built the extensions framework
  • m_conley: Any other questions?
  • justy777: Not at the moment
Advertisements

One thought on “Meeting Minutes: November 16, 2014

  1. Pingback: Status Reports – November 22, 2014 | Review Board Student Blog

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s