DFFML Maintainers, 12 Oct 2021

Previous Meeting

Next Meeting

⏯

youtube image

►

From YouTube: Maintainers: Code Review

Description

From Weekly Sync 2021-10-12. Went into detail on how one can do code review. Notes: https://docs.google.com/document/d/1_Vxmlgh_MMdAhyGqRKSsgyaXOM620oV_r5bSfQFByBE/edit

References:
- https://www.morling.dev/blog/the-code-review-pyramid/

A

Okay, so let's see what we've got here, so they added an entire plug-in um and.

B

It looks like okay, good, good, good, okay,.

C

We can join another meeting if we like.

A

B

Let's see so for sure contains comments, bull, true.

A

Okay, so as far as um obviously, you guys know that this is, um you know, I would say, lowercase this rate. um So, let's lower cases.

A

um Also, that's way too long.

B

A

We could say, for example, you know this might be a good thing for field. So so what are we looking at here when we're doing this? Okay, so essentially, um most of what I do when I'm reviewing code is thinking more about, like mostly about maintaining the code going forward, um because the thing is the code is always going to change. So, what's most important, in my mind, is making it so that there won't be large problems. If we do change it, you know um it's because we're sort of assuming you know we assume.

A

If there's one thing, that's true, we know it's that software has bugs right, so we're assuming that the code will will possibly have bugs. So how do we make it? This is sort of like my general mindset. How do we make it so that there's less problems when there are bugs? So um so? Basically- and then you know so so that goes into like you know, readability of the code um and then things this is sort of more of an end user perspective thing. It's also like a python style thing uh we wouldn't use.

A

You know we wouldn't use all caps uh variables here and the other main thing is consistency, so um you know the the and the code base you know strives to be more consistent than it is the config stuff is why I was so mad about that is. We need to make that more consistent, um but um this is and also as sort of as it relates to consistency.

A

If something is consistently wrong, then we know that we can change it in the same way in the future right. So if we're going to do something whatever it is, let's be consistent right, so this rule applies here. So these guys there's an inconsistency with the capitalization.

A

um So let's, let's lower case it, um and is there a shorter, let me make sure we're recording. Okay, so is there a shorter? um You know a name that we could use this name that we could use for this.

A

We can put the full description in the uh help text.

C

It can be like header equals to true or false, because that's how pandas did it.

A

Yeah, what was it header.

C

If you have a reset which we don't have.

A

It's false great.

A

uh Yeah and once and that's a perfect, that's also a perfect example of like okay. If there are ways that you know that other people already know about doing things choose those ways right like invent, invent new terminology and new uh stuff as little as possible, and and and try to stick with things that that people know right. um You know unless you absolutely have to sometimes you're in a space where you're doing things that you, you know: you're you're, it's exploratory work and there's not really good names for things or there's. Not really.

A

You know you know, there's not really something standard or something else that you can fall back on right or maybe you don't know about it, um but do your best if you do know about it, to try to make sure that it stays consistent um default equals true, all right great. So uh the other thing that I do when I'm doing pull requests is so I have found. This is more of a me thing, never mind. um Let's see so I was what I was going to say is.

A

Basically, I always hit comment and not start review, because, as you guys know, I forget about things and and then you don't have- maybe almost all of your review, because I forgot that now he's gonna make one more comment. So, um okay, this is another thing. So if we change this up here, I try to resist the urge to go through and change the rest of them, because that is not a good use of time. They'll have to do that anyways.

A

So, let's see so load fd file, object.name, okay, so right here, we've got an issue um so and what does the ci say about this? I don't think it's going to get run in the ci. Is it.

C

Not unknown we'll request something like that has happened.

A

Oh yeah, so the other thing is so this this one won't end up getting run in the ci because you have to add each plug-in to the matrix, and so this one never got run in the ci because they didn't add the plug-in to the matrix.

A

This is another problem that will be fixed by moving to the second party stuff so and also our ci job times are going to go way down so or will they go way up? I guess we'll see dependency dependence on the dependency tree of the things you're changing so fileobject.name is not something that you can rely on.

A

This probably worked with a regular file. Let's see what did we test with async test case test, xlsx source, okay, so temporary directory for testing? Okay, tester, random? Okay, so we open the file okay. So this probably worked fine because we passed in a real file and so the file object handle um the the handle different source file.

A

So this is the file you know the base class here for the file based um stuff, so the call to open, since this is a regular file, we're going to hit the call to open which is going to give us a file object with a name attribute.

A

But if we had ended up with a call to like the zip file helper, we would not have a dot name attribute. So, and this is, uh this is one of those things that is caught, um so we know for how would we? So? How would we catch that? This is possibly an issue right? Well, we know that we're implementing off of the file source based class right, so we know that we're going to call load fd.

A

So we know that you know we should okay, so load fd comes in. Where does load fd get called so load. Fd is here right, so then this is how we would see that, and we would see you know that you know as a reviewer.

A

I know that we're not testing the rest of these paths right um because I see that we're going obviously we're only testing with one to file type right um and the base class is going to call the load fd based on you know, with with it, it could possibly accept other file types. So we have- and this is another thing where so the coverage information, so that's another thing that we're going to get better with the second party plugins is because we have the coverage distributed across so many ci jobs.

A

um You know we could think might just figured out a way that we could fix that, but we needed to move to the second party plugin, so we're going to have better coverage information now because we'll actually have it on each one. Did it kick us off.

B

Let me see if I can share again.

A

Yeah we're gonna, I'm I'm. I bet you revolt has video.

A

Let's see okay and I got the paper. I was just looking at a package manager the other day that is interesting for linux, distro.

A

Okay, okay, let me let me focus um okay, we'll do that later, so, okay, so yeah. So basically you know that thinking. How would we catch this bug right? The first thing I'm looking at is: are we looking at an object right, we're looking at this object? Where we don't know what the properties are right? We know. First of all, this is not a file object right.

A

The method name says load fd, so we should be accepting a file descriptor right, um which maybe our tutorial says, file object, um but it's a file descriptor right and a file. Descriptor is um well, it should be an integer, but you know we're assuming it's a basic like a generic python file, descriptor right, which doesn't necessarily have a name attached to it. um So we talked about coverage.

A

Essentially, you know you would see this we're we're looking in here, because we know that we've got a base class involved and so we're thinking, thinking, okay, well, how? What is our code path to load fd through the base class and we've identified you code? Cov will tell us this once we have the second party stuff, but from a general perspective, if you didn't have that, you would know that you need to trace, through the base class, to identify possible issues with that code path.

A

Okay, so we can't rely. We can't rely on file object.name existing, um so we need to, and then I like to try to ideally try to link two code examples. So.

B

So, where is that so source.

B

B

Okay file exists; okay, so.

A

The other methods of opening a file will not are not.

A

Not guaranteed, however,.

B

You spelled that.

A

Will not always um give a dot name on file object.

A

Okay, so we should so load workbook, so essentially we're saying yeah, so we're saying: okay: where is this load workbook? So now, let's go check this out come on man. My computer is just being slow today, okay,.

B

All right, so is this the right thing? Yes, it is okay. This is not.

A

What I want, uh where is the real api docs where's, the we know, where's the source code, always look at the source code, it's better than like, if there's api, docs great, but what's even better, is looking at.

B

The source code.

A

So yeah first thing check the api docs, the api docs. Don't answer your question immediately, then you know.

B

Jump to the source code, so example where's the api. This is an example example. There we go okay,.

A

Load, workbook read-only file, name so file, name string or file like object, open in binary mode okay, so we can pass it directly. A file object so we'll link to this documentation where we found that okay, but this is also a reader. So what about the writer? Because I think we're supporting writing here.

B

So, okay, where is load workbook load workbook?

B

So what if the workbook doesn't exist.

B

Who knows? Because I could tell you it doesn't exist right here so allow empty true, so it's gonna so save so. This must load workbook reader, workbook.

B

A

Don't think that reader is the.

B

Correct one, I don't know if that's one we're gonna load workbook, is that, where we're gonna end up open the file name and return, the workbook.

A

uh You know this is, and this is where I would basically say: okay, so at this point we will see right, so we found the documentation the documentation, let's look at the source while we're at it. The documentation says this is a reader yeah, I'm not I'm not sold on this working, but the documentation says it will work.

A

So at this point I would say we have to make the code change and if it doesn't follow the documentation, then we make another change. But you know this is sort of one of those things where we should probably just change it to file object.

A

B

Okay, let me just get a name on file object.

A

So uh then we would link to the appropriate documentation where we found that so that they know what we're talking about.

A

So the load workbook.

A

Method function, not that it really matters um function, um says that it can take, and then.

B

A

Quote, string or file like object or.

B

B

B

A

Like to just put this references, let's see, let's try to pass the file object.

B

B

Let's see if that works.

A

All right so at this point you know you can't know everything. It's the thing right. You can't like you can know some things for sure, but you really don't know anything until the ci runs right so provide what advice you can right provide what information you can and then you know sort of you know. Then you can reevaluate once you have new new information right.

A

um So then, let's see.

C

So so what do you see as a potential problem? uh How do you force your potential problem in maintaining stuff like how some part of code can be potentially be a problem in future? Okay and.

A

C

You foresee that, like that is a question.

A

So, for example, whenever you have multiple code paths, um so these things are examples of possible problems um like so you have. You have two code paths through this right and they're very similar, and this is where you you you, you know you could. You could quite likely end up with an issue.

A

So how might we rewrite this um so that we end up with so because anytime, you have multiple code paths you have uh so, for example, if you had a change in the future where you would need to change this line, you might forget to change this line right and so any time where you have you know repeated code, try to reduce that. So, for example, I would say- and there's also you know- there's many there's many schools of thought on this.

A

So there will be people that rail against what I'm about to do, but um so some people will tell you that you should not use generators and you should not use turn ternary operators, but I say, make less code paths um so because the thing is, there's there's also there's this trade-off between readability, right and um and because your code right, your code's gonna, be read more times than it's written, so you need to make sure the code is readable, and so, if we we have a decision here, we can have an if else block at this outer level or we can have an ill splock right here right.

A

So I would say that first of all, I would say people did the argument that you shouldn't use generators because they're not readable if you're coming from other languages, I I say that's unfortunate. However, all languages have quirks and if we write everything the exact same way, then you know there's no point in having different languages.

A

um This is why they all have their specific features. So I would say- and let's see we change this to header.

C

For people in python it is much more readable like it. It.

A

Is yeah exactly if you operate in python you're going to say uh you know this is going to be very native to you and if it's not, it will be the more you read it right and- and I understand that that is you know- maybe not the most popular opinion. However, uh the language is just you know, the the language is just the language is not the language that you're writing. It doesn't really matter.

A

The thing is everything is just api calls right, like you have syntax right, but syntax is mostly the same across languages right and you can have this generator in python and you may not have a generator in another language right, but you know that you that you know how to you know it's just translating right.

A

This could just go up here right and then this would go in that right under this still, you know you you have to um focusing on that uh sort of you know, misses the point of if you're in a language use the language right, um okay, so basically cell value, if what is this otherwise cell letter right? So I would say that this is an example of a place where you know we detect a future problem. The code works fine right. This is great. However.

A

Now we have less code right and less code is always better or well, not always better, but there is there's a trade. There is one two three four.

A

There is a trade-off to be made with readability right. um I would argue that this is just as readable. um You know if you get, if you get to the place where it's not readable, you need to write your own function. It needs to be its own function right and then there's also sort of uh yeah. Okay. So, let's just say um let me just let me finish this. So basically, let's say um let's say: uh let's condense, this.

A

Into one statement.

A

Okay, so the and, like I said the main reason, the main things that I'm looking for are um like. The main thing that I look for is: if I'm going to change this code later, how much? How much of.

C

A

Is it going to be um so, let's see okay. So what else do we have here? So mamster row record roaster row? Okay, so it looks like we might be taking the entire row and making a string out of it. So that's a red flag, um because obviously, let's see or well what is row. No okay row is an integer all right, we're fine. There.

A

Okay- and you guys also know- I obviously really like generators, so that's a bit of a biased piece of information, but um I think that they're, nice and they avoid giant for loops with variables where you are redefining things and yeah. um Okay, so row self column features row plus one.

A

I don't like that. um So comms feature feature ws, so where's ws.

B

A

Worksheet, okay and anytime, you have this type of thing, for example this this um yeah. This is another thing, uh that's just sort of.

A

This is more of a a choice right so.

C

This can be done. The ternary operator right.

A

You could do that, um however. That's that's so okay, so you could say it like this and write it all in one line. However, I would argue that this is more readable, um because you have your default case and then you say your exception case right. So this implies this this. This does not imply that there's a default right. This implies that there's a possibility. One way, there's a possibility. Other way right, but sheet sheet has a default value right sheet or no, it doesn't have a default value string.

B

Okay, well, if, if a sheet is specified.

A

Okay, so, but that that is sort of a default. So if there is a specification, so in this case- and you don't always end up with this right, so sometimes dot active is like a property that will do something. In this case I would say this is the default. If there is a sheet, then select the sheet wait, does she have a default?

A

No, okay? No! She.

B

Does have a default.

A

The value of none right, so this is a good example of default value. Otherwise, do this right.

C

Start saves as that's it's the it saves that looks like a elegant flower. It is going in right.

A

Yeah, okay, so and then the other thing I like to do is okay, so this is there's a few things with python that sort of uh there's a few things that are python specific, that okay, so there's a trade-off between verbosity and um language features. um So so, for example, we were just talking about generators right. This is um this: is this generator doesn't use any funky python language features that you might not have anywhere else right? This is a for loop.

A

You've got the inner operator which yeah you know some people, don't some languages, don't have that, but it people generally know what this means right um now, none. So none is a python. You know things like undefined, for example, in c you have. What do you have? You have null and c um you have undefined in javascript or null in javascript and then in python. You have none, um and you know, take your pick from the other languages, but usually things have some equivalent of null right.

A

um So none is one of those things where if, for example, okay, so if I'm coming in where's that javascript thing okay, if you're familiar with javascript, you know that there's an equals equals and equals equals equals right. And you, I'm sure, hearing that say, that's nonsense. Well, it is a bit of nonsense.

A

One compares the type and one compares the um one compares the type and one compares the actual value right so think about ifconfig.sheet.

A

So config.sheet is none right. So, but none is not a string, but this if statement will evaluate to the same thing if n, if sheet is set or if sheet is set to a blank string right, and so you could argue that one means that they mean the same thing, but they don't mean the same thing. So if config.sheet is none is when sheet has not been set right, but if config.sheet means okay.

A

So if config.sheet is not none, so this says, if config.sheet is set to a blank string, I should still treat it as if it's a you know as if it's been set, if you don't say is not none, then you're going to be treating blank strings as if they're none, and I would argue you should avoid that behavior. Unless you explicitly want that behavior right um and we haven't, we don't know whether we explicitly want that behavior or not.

A

Here I would say that um it's better to be explicit and treat none as if it's your default or no default. um So if there's not none does that make sense.

A

I know that some people may not agree with that, but I would say that it's better to be more explicit about uh the fact that your defaults are not default. Now. I think a better option is to do kind of like what they do with.

C

Yes, because a lot of things can't be truthy like uh you can pass through that stuff and it's not necessary that.

A

Exactly yeah and you don't know, what kind of call stack is gonna come all the way down right. You may you may end up like blank string. Likely is not a valid sheet name, but if it is a valid sheet name, then all of a sudden you have an issue right and you're, not supporting that valid sheet name right. um So what I do? I love this. What they did here.

A

Okay, this I love this, so this is great. um So basically, what they've done is they've defined this missing type thing and then they instantiate a instance of this missing type and and they also do this for like default and no default, and that way, if you have a value of none like if none is a valid value, you can compare to say if field is missing and you don't have to deal with the fact that you know the field.

A

Might you know you're never going to set the field to missing, but you might want to set it to none right, and if you declare this specific type, then you can actually have something that allows you to have none as a default or as a set value. um Okay.

A

So what else? Okay? So we simplified that we got rid of some lines there we did sheet as a default. This all looks good, so, let's just say so, columns feature where's columns. Okay, this is columns if.

B

B

Columns if cell value.

A

Record column names for convenience else, column.letter letter okay, so this just creates a mapping of letter to letter for the first row. So is this the first row? The other question is: do we are our index starting at zero here, because right now it looks like we're not reading row zero.

A

So worksheet, let's go look at the documentation, and this is another thing where you know this. I would not go okay, so the other thing is. I would not usually go this deep right, we're doing this now because for the exercise of doing this, but this is the type of thing where, if I think I'm wondering what about one right, I'm I would I'll okay. So I would say if I'm wondering what what happens with one should one be one.

A

I'm just gonna say you know what it doesn't matter, because we're gonna make the changes and we're gonna run the ci and we're gonna see what happens right so because there's a chance that yeah they're perfectly right, maybe the the wavy it starts at one right, um but I would bet that your headers start at zero, um but.

C

You know, maybe this is zero one, maybe in the head at one point.

A

Yeah, I'm that's my guess. My guess is that the zero with row should be the header one right, because max ws max column, so this is max column plus one. So it's this and then the other question is max column. You know, is this something that's set based on? What is the is this the maximum column in the worksheet, or is this the um or is this the last defined column right?

A

You know the last column with any data in it.

A

I have this bird that keeps coming and knocking at the back window. um Let's see, let's see uh yeah so but, like I said okay, but it doesn't really matter because we're gonna find out when we run the ci. So always remember- and this is the thing so this- and you guys know this in our code- reviews that we've done together. The unfortunate part of this is now we're going to have some lag time between the two of us. When we do the review of, we have to look at the ci now.

A

The thing is, I could spend the next hour trying to figure this out. If you know for some of some issues like this, I might have to dig through and run things and it just it just doesn't make sense right. We can ping pong back and we can see the ci. We have to figure out. How do we use you know both of our times most effectively right.

A

If we wanted to, we could spend all day call tracing through every piece of source code, that's involved, but you have to make a decision of what am I going to find out anyways. You know um so yeah, okay, so this is like I said, we'll find out um we'll look at the logs, we'll see what happens, um dump fd initialization dictionary, a file does not exist, create a blank work book.

A

Okay, try, self.wb, okay, so basically they're trying to access this parameter. Don't like that at all. I don't like this. These things should be anything, did try, try catch block, not a fan.

A

There are methods that do this type of thing. So, basically, if has at herself.coms and also.

B

A

So you also should okay, so here's another thing, so anything that's set on self really should be set within a nit and it should be set to something that maybe declares it as, for example, okay. So, for example, this is a classic a inter a exit or inter and exit thing, but we just wrap some of that stuff and so we're in load fd and dump fd. So I would say that you should set self.com where's an example of this.

B

A

I mean I'm sure that we're not following this advice somewhere too, but since we're trying to figure out everything at the moment, um let's see so.

B

Jason, no, he needs any.

A

Could any do this and he's not going to do this? um Where does this? Oh.

A

B

Until I sync test case.

A

A

Okay, no, no okay and here's an example where we're not doing it too. Okay. um So what this should be is self.stack equals none or dell self.stack. um Actually, probably dell self.stack and dell self.a stack right, so this gets rid of them. This will unallocate them this allocates them, and then I would say that def init.

A

You should really have them initialized to like none.

A

This is this would be the uh you know. I want to say idiomatic, but I'm not sure if that's the right word. This would be like the way that you're supposed to do it right. You initialize everything in a net. You might use it somewhere else and honestly, I'm not sure about delling it, but you could do that. uh It's pretty much the same thing about as you. I often set it to none and then I'll do a check to say. If this thing yeah, you should not deal it. You should set it to none.

A

um Now I hate this because then you end up with the same code. Two places right, um but even it's just two lines, but it's annoying. um So then what you can do is- and this is a common thing with context managers like this- is you might check for re-entrance, which is basically trying to call enter a enter twice and some context managers support that like and some do not right, and if you write your code like this, then you can say you know.

A

If self.stack is not none, there's code somewhere in the code base, that does this, uh you know raise no intru. No, uh like you know, context manager.

C

Simple model does that, like a simple model, yeah that kind of thing that, if it's already in context, don't go in context, thank.

A

A

Yeah there you go yep there, it is yeah. Thank you. uh I.

C

Worked on this phase.

A

So yeah, so this is and yeah there's in context. So this is exactly what I'm talking about, so we define it here, check it here and then you know do something with it here to reset it right um yeah. This is an interesting, uh let's see so anyway, so that's the paradigm there. um So I would say you know here. Basically uh we should say, and what is this so this is a mod. This should be a context.

A

No it's an xls source, so I believe an excellent source. Okay, so in file source. Basically, does this stuff by faking the context and having the context be itself.

B

The informal source file.

A

Also, should this be a binary file source, it probably should be a binary file source, because this is not a text-based file, so uh you know this should probably be binary file source.

B

A

Binary file source, okay, so here's the other thing: god damn it. uh Okay,.

B

I hate that okay, so.

A

Okay, uh here's the thing is.

A

uh Basically, what I'm trying to say is classes classes are not great, um I don't like them because they hide stuff right. This is why uh I'm a big fan of the data flows and stuff right. It hides less stuff, because you're you're going from function to function right, there's still some stuff that you don't see. But anytime you have this inheritance stuff, you you end up sort of hiding things right and then it's you're, not hiding things.

A

It's just hard to tell that this exists like how do I know that this is here, and I want to use it right, and this is part of the motivation behind all the data flow stuff is okay. Hopefully it becomes easier to figure out that hey, I have a binary file. I want to use the binary file operation to open it and then I'll pass it to my load. Fd right, that's sort of you know meta, prod, project, wise.

A

Why why design decisions, um but if you're working with classes, then you need to make sure that you go look in the classes right. um So then I would say this looks good. So basically, this, uh let's define uh self.columns.

A

Let me just link to that. Actually, since you, you posted that, so let's look.

B

At dfml uh model.

B

Model I swear there's somewhere, that's a little a little.

A

More just like just the set and admit you know, maybe we'll just copy this.

B

Here's that config, okay.

A

Did I only get the enter okay.

B

Okay, so uh this is, you know, load of tea.

B

And we're looking at context here or columns.

B

Okay, so dump 50.

A

And you know we may not actually want to reset columns on dump fd here.

B

If self.coms is none.

A

Then self.com is I don't like that.

B

I would say.

A

This is a good example of sort of that. First one we went over with the. If else, so I would say um columns.

A

Wow equals this: if self.coms is not none, then you know columns equals self.coms.

A

Right now, you've got your local variable to work with.

B

uh Self.Com, so we should probably set such a cause, equals and subtitles.

A

Okay, let's define columns.

A

In init, oh- and I find that using using the markdown formatting, I think really helps um so I I think that it helps people understand what you're talking about when you format it correctly. So it's worth trying to do um so. Okay, so celticons.

A

Let's define self.coms and init.

A

uh And avoid using a try catch.

A

Okay, yeah just try blocks should really be avoided at all because um they should really be. You should really treat this as sort of like.

A

If you know exactly how you're going to handle an error, then do it right. um For example, name error, attribute error. Okay, so like that that's this is a good example where ah okay, this is well. This is a bad example. um Load workbook.

A

If you had something specific and then you did something specific, that's good or if you're doing something generic, for example, the data flows, they don't know what's executing right, so they have to have try except well. Actually, I think they actually handle the error without the except block due to the async io stuff, but generic stuff, where you don't know what your error is, yeah try accept block. You know. If you do know what your area is, try accept block, otherwise really try to validate your data beforehand.

A

So you don't end up in a try except block.

B

Let's see that looks okay.

B

Calms: okay, config.

A

Sheet, okay, so we have, we need to check, um let's see so this uses sheet without checking, if it's none we need to ensure sheet is not none for creating.

B

A

um Okay, yeah- and here we may have just once again said this type of thing this. Hopefully, hopefully they will see this comment and then move this logic. You know up here.

B

um So, let's see this check system.

B

A

ah I need to ensure that two is not done before, creating it. Okay,.

B

B

Yeah, so let's this.

B

Workbook.Sheetnames.

A

Equals zero- and this would be this- is a list then there's no reason to get the length of it. We can just say if the list is empty.

A

Now that is more of a pythonic type of thing, but there's definitely you know so, like I said basically trade-off between language features. Right. You know, there's there's an aspect of things where you really want to be. You know you, you want to try to be true to the language and then there's sometimes where you want to try to be more uh generic. You know, depending on it really depends on how how unfamiliar is this going to be to other people right?

A

If it's, if it's, if it's going to be, if you think it's within reach for people coming from other languages, you know definitely do it if you think that it's going to be out of reach, for example, operator overload, I hate operator overload because you don't you have to you, have to know that you're you're, you see two objects and you're like great okay.

A

Now I have to go, for example, when you're refactoring, if you use operator overload, um you now no longer have the ability to do like a find and replace on the methods that should be, for example, doing the add operator on two objects: um try that's like an example of trying to keep, for you know to get away from that type of stuff.

A

um Okay, so set desired sheet is active. I think we're almost done here. um uh Great.

B

Okay, so create blank sheets, no okay, so.

A

We create the sheet we should check. If sheet is none, so if sheet is none, do we have to create it? This is another thing where it's like. Okay, now we don't know the api. um So is it a possibility? Let's look up here, so there's no sheet, so basically we assume that we're grabbing the active sheet and that's in the case of a load.

A

So if there is a workbook will it have a sheet? Let's check it out.

B

Simple usage write a workbook.

A

Okay, so it's got an active one, so it looks like we have an active sheet, even if we, if we select active, but we haven't yeah, so we select active, but we haven't called create sheet then so we could oops so select active, but we haven't called create sheet. We will end up with a sheet anyways, so in that case, okay, but they're saying if no sheet names.

B

B

A

See what they're.

B

A

So that desired sheet is active, so if you have a sheet, if you want a specific street name, you should create it before you select okay, this, but what's going on here, because this is saying config.sheet- and this says, if not sheet name, so we should really.

A

We should really be checking if this should really be a check for.

A

A

Not in sheet sheet names right, so basically what we really want to do here is we really want to create the sheet if it does not exist right and then we really want to. It looks like we want to create.

A

All right, so it looks like we're trying to create the sheet if it does not exist. So let's make that what we're doing, and then this should be simplified here, because we shouldn't end up without, and this is once again so I would say like what's going to happen here. I don't know what's going to happen here is the problem.

A

So I think this is a case where we don't want to do that default behavior. um You know the default behavior thing that we just did here right because I don't we don't know what happens create sheet.

A

Is it a sheet? We don't know what happens when you do dot active and you haven't created any sheets like we don't know what name you end up with on this sheet, so I would say that to be safe, we shouldn't do the defaulting. We should make sure we should preserve the if else block right.

A

So and then we should say so. If- and this is I would say I would write- is not none, so I would write your okay well, maybe yeah. I would write your non-default first, because this is okay. If I'm supposed to do something with this variable, then do it otherwise drop through right, so sort of like you've got a switch statement in something like c. You know you're sort of what happened here.

A

You're highlighting is off. Why is there.

C

You know python 3.10 has introduced a sweat statement.

A

Oh, they did oh yeah. I think I read that yeah. Oh, my gosh, I'm excited it's about damn time. um Okay, so, oh that's great great news, so I think this is probably satisfies our thing um and once again you know what happens if we do dot active, we don't know. So, let's make sure that we do. You know, let's make sure, because my guess is, if you do dot active, it's going to give you a default name right.

A

So if the first thing you access is dot active, you're, going to end up with a sheet with a default. uh Otherwise you should really just do the sheet. Okay. So then, here basically okay. I look at this wall of text here and I say great I could go through and I could trace through every line of this. But what is this supposed to do?

A

This is supposed to write out an excel file, so I'm going to say, run the ci and if the test case looks sane, I'm going to know that that block of code does what it's supposed to right. So I'm going to go. Look at the test case and I'm going to say I'm going to say that this.

A

A

None none is expected behavior, because columns are empty. I don't like that. um That should be configurable. So.

A

Okay, but this is a bigger problem, so this is one of those. This is a sort of bigger fish to fry type of thing, so this is one of those okay, so I would say that, oh god, this is annoying. um So, okay.

B

A

Do we deal with this, so this is one of whenever you're reading and writing things. You're gonna do this crap. um So we we, if we read out not as expected, behavior because columns are empty, and this is we've already written a file.

B

Save and load so what happens if I save.

A

So if I put it in there, okay and now so now, what I'm thinking about now is we have an interaction with the memory source. I think we're about to run out of time, but I think we're almost done here and we'll just call it if we do run out. So we have an interaction with the memory source here which might give us inconsistent behavior, so the memory source basically caches the stuff in memory.

A

Until you know you do the intel until you exit the double context, entry right. So if I did this assert dict equal here, um if we had the source open, for example, within a double context, entry- and we read the features of a we wouldn't have c and d- uh you guys see what I'm saying, because c and d are populated only because they existed when we added record two and we read both both records back after having the same voice.

C

Yes, does that make.

A

Sense yeah, so this is a possibility once again possibilities for inconsistencies right, so you know, inconsistencies encoder, inconsistencies in behavior, right and so there's ways. There's multiple ways to deal with this, um and I would say one way is to this is this is one way make it consistent that can sometimes be incredibly difficult or not possible or something that you don't want to do right. Obviously, like we're not we don't know that c and d exists.

A

If we checked it after, we inserted record we're not going to know that c and d exists until we reload the stuff. If we were within an entry, so it's not gonna, there's not a way to make that consistent. So none the other way. You know the other way you can deal with it is to document it.

A

If you have inconsistent behavior, it's it's it's okay to have inconsistent behavior. So long as people know, what's gonna happen, you know in different cases. So in this case.

A

In this case, like I would think about the use case here and the use case is probably read a bunch of records in from an excel file and do some machine learning and dump out one column, that's probably going to be your prediction column um and what happens if you have null values so.

A

I think none is a good thing to have there. I think that it could should be configurable, so you might want to read zero or something instead. uh The other thing is that doesn't really matter right. We should probably be feeding you know you may want to actually just feed the stuff they're pre-processing source anyways.

A

So from that perspective, none you know who cares if it's none, we can replace none with zero uh when we're pre-processing with the data flow. So I think this is probably fine for now. um I think that we should have something that says that it will replace.

A

I think we need to document that, um so I would say you know the class doc string. Okay, so.

A

You know uh so, cells within cells with empty values will be treated or.

B

Have future data for that cell.

A

Set to none okay, now, we've documented it so we're good to go um now. Will somebody be mad when inevitably they're like? Why is my thing? None, yes, always we're always mad when uh things don't work on the first try, but we will at least have an explanation for them. So um and then what else is there to do here? So the last thing is, you know I'm not seeing features or predictions, and this is a constant thing that we have with all our sources, which is really annoying is like.

A

How do you deal with predictions? um So I think we dealt with it recently, so we need to figure out. This is- and this is mainly a so reading and writing thing. Where did we deal with this? Do you guys remember?

A

um I don't know if it was one of you that had to deal with this recently, but let's find out- and this is the other thing- this is my new favorite thing. um I think I've been preaching about this to you guys, but this git log dash p.

A

I mean this thing is just the best, uh because you can just look at every commit and you can look at specific files, and you can say you know what happened in this file or what what what happened in this directory right um and um it provides you with essentially a set of examples in a lot of ways.

A

So what do we have we're? Looking at predictions? Okay, so data frame source? ah This must be where it was recently. So who did this.

B

Oh yeah sure, okay.

A

Okay, yeah the data frame source, that's right, um so this is an example where we had to do the same thing where it's like. Okay, you know the data frame looks very much like a csv file or an excel file. So how do you decide which columns are predictions, um and this is also related to hashem your confidence since prediction? Splitting pr um did we just get dropped again?

A

No, okay, ten minutes, all right! We're.

C

Gonna wrap up here.

A

So, but this is related to your confidence and prediction: splitting pr is like okay. Well, I you know, I would we have these predictions.

A

It makes it it will make it easier when we have that pr merge, it'll, make it easier to say predictions and then confidences or something right, and so then we could potentially because right now, it's it's kind of ambiguous how a source should deal with reading and writing predictions and their confidences, because there's two values associated with the prediction right um and so what we did in the data frame source was just list.

A

You know these are columns which you should treat as predictions, um and that that I mean I don't know if this is the best way to do it, but it is one way to do it and it and- and you have to know your predictions ahead of time, um but you know this is one of those things about the separation between models and sources and actually scores, and all that is trying to think about what data. Should you know right if there's a possibility that you don't know a piece of data for some plug-in?

A

Maybe it should go in another plug-in, or maybe it should have its own plug-in right, because we need the plug-ins. You know the plug-ins are specific to the data that they're working with right. It's an implementation, that's specific to the data it's working with, and so I would argue that the source would know what you know the source would know likely what the what the predictions that should be like what what we should treat as predictions out of the source right um and so this this is. um This is how we do that right.

A

At least this is how I did it with the data frame source. um So I would say we should follow that same pro pattern here. So I would so I'm going to link to the data frame source and I'm going to say.

B

Source data frame.

A

B

A

Okay, yeah, so I would say uh predictions.

A

Okay yeah, so, let's just say, let's just say reference the data frame source and I think that will be good yeah. Okay,.

B

Okay, so, where.

A

Is that so, and I would say the way that I would phrase this is if you know that something's going to be needed in the implementation, so, for example, I'm what I'm going to say back is: let's make sure that we've tested this. Let's make sure that we've testing this with predictions in records.

A

It would be a shame if we've made a bunch of predictions with the model and then couldn't save any of them to the excel file. So this is and then you also need to make a decision on what is a gating factor to the pr and what is not, for example, predictions. Definitely a gating factor. You know the source is not really usable, I mean it is usable, but I would argue this is you need to make an assessment?

A

A judgment call on what is it okay to merge and then create as an issue and track the issue and what is it? Okay, what must be done before we get the thing merged because you're going to get a there's, a huge drop-off that happens right and- and we all know this right- you're working on your pr you're working on your pr, you get it merged and then you're like okay.

A

Now I have a million other things that I have to do right and chances are you're not going to get back to those issues that you were supposed that you need to fix as follow-up issues for a while. So you need to make sure that if you're going to merge a pr, you're, okay with it, not getting fixed for a while, if you stand, if you create issues and say fix those later, um uh you can use the data frame.

A

The data frame source is a good reference. So any questions comments concerns on the code review that we just did.

C

It was really nice like. I would also go through this once more just to like collect the points.

A

A lot of points were there: okay, I'm and I'm happy to do this again. Obviously this took a little over an hour um code. Review takes a long time right um and.

C

Especially when you have to be purpose about your thought process right.

A

Yeah yeah yeah. I was trying to give you guys my whole thought process here, and but you know this pretty much is how long it takes me in general, um because there's a lot of clicking around and stuff, and you have to dig through source files and it it. It takes a long time if you're gonna go through it and really, you know, try to provide a lot of feedback, and the other thing is this is why we're so religious about all these ci jobs right? It's because, for example, okay, we need to go.

A

Do the I sort thing, because this is a good example of where isort would be fixing this type of thing right or telling us that that's a problem, because we want to just make sure that the code review that we're doing is really it. It should be less about the functionality. The people implementing stuff know they're implementing it, because they know the functionality right. What we're trying to do is is help them make that functionality.

A

You know accessible to other people, you know and accessible via the interfaces, they're, exposing it and then maintainable long term. Since it's a contribution right and and let them let them be, the expert in the way that they write out the excel file right, um just make sure that it conforms right consistency with everything else. Okay, so the data frame source is a good example.

B

Of how we handle predictions for something with columns.

A

Okay, okay um and I think that might be sufficient. Let's see, let's make sure we're testing my predictions and records okay and then let me probably link to the date now they can find a test for that.

A

Okay, all right great, so I think, but this is uh this is a long meeting today, thanks for sticking around guys- um and it was you know it's great talking to you guys and uh we will get so I'm I'm going to I'm going to do the second party thing by next week, um because or else I'm going to be stressed out that it's not done um and I'll look at this package manager paper here.

A

Let me oh that doesn't really link me to it that will copy link address and then um and then I I'm pre, so I'm probably which what I'm saying is, I might be slow I'll, try to go through and do my five issues like I was talking. We we talked about last week need more info, uh but but maybe I'll I'll definitely do one.

C

Of us to pick something up that might help you in your whole process, something like that.

A

Okay, okay, yeah I'll, keep it in mind. You know.

C

A

I'll ping, you guys um okay, so we did pr review uh and then infra and we'd talk about that some other time uh because yeah, I saw what you said and it sounds like we're. Basically, in agreement on or well, we both have the same understanding about pulling out the tokens.

A

Okay, we'll figure that out, though, that's sort of a long term thing, not super critical, um I'm you know we'll run it off the droplet that I already have for a while and then we'll figure it out.

C

Sure you will start using brave and then you'll never be able to go back.

A

Yeah exactly yeah, I'm hoping so it looks good and then we can do the meetings in there and stuff hopefully, and we can set up everything and yeah well sweet all right, good talking to you guys today, and I hope you have a good. You know rest your evening and uh I'll talk to you next week.

D

A

Thanks have a good one, bye.