|
New Yorp New Yorp posted:I'm not a fan of either approach. Code review isn't the place to be ascertaining functional correctness -- that's what unit tests, integration tests, and QA/UAT testing are for. Code review is to catch obvious mistakes, style issues, horribly inefficient algorithms, etc. There is a QA process happening in parallel in either example as posed here. Not being "production ready" means that features added there aren't necessarily complete. There must not be any regressions, but the new code--if it's scooped up in a release--isn't advertised to end users and isn't normally accessible. There might be some special flags to turn them on for a target user requesting a particular feature.
|
# ? Feb 1, 2018 21:25 |
|
|
# ? May 27, 2024 02:58 |
|
Rocko Bonaparte posted:I'm looking for some opinions on the scope of commits going into code reviews. If it's large and architect changing like that a hybrid is usually the best approach. A series of commits in a single review that walk through the steps and can be viewed independently or as a whole as up to the person doing the review. Prior to merging this can be a final squash or not depending on local taste, but I'd rather not squash personally. It looks like in both 1 and 2 you're saying that one commit = one code review. That doesn't need to be the case. I can have a code review of 40 commits and do a diff from master to the last commit and get the whole view, or I can dive in and look at individual commits if I get lost, either way though I'm going to comment on the whole thing not on the individual pieces of it since that's what will become master.
|
# ? Feb 1, 2018 21:38 |
|
#1 sounds awful. It's impossible to meaningfully review huge changes. Reviews should be about fulfilling one part of a functionality and ideally be ~100 LoC so that a reviewer can actually fit the change in their head.
|
# ? Feb 1, 2018 22:24 |
|
Portland Sucks posted:edit: To be honest you don't want to be needed. You're working under bad management if anyone is full on needed. How can you take a vacation if you're needed? This has been a really hard one for me. I tend to work at small places and therefore am often needed, which has come with lots of problems. Few people can directly help me on problems, because few people means little overlap, and everything is critical all the time which means if I leave everything breaks or stops. At my current job I was spiraling into extremely bad burnout due to this problem, I had to sit down with my bosses and say basically "either I take a long vacation right now or I have to quit, I'm losing my mind". So right now, I'm on a vacation - my first one in over 2 years. It's been a really hard lesson to learn but I think I have finally learned it. I'm never going to let myself get in that position again. To anyone else who is dealing with a similar situation, please make regular vacations non-optional, no matter how much there is to do. I had the same problem Pollyanna did where "open vacation" meant "not now, it's too busy", which really meant "never". Gotta force the issue.
|
# ? Feb 1, 2018 22:36 |
|
Taffer posted:This has been a really hard one for me. I tend to work at small places and therefore am often needed, which has come with lots of problems. Few people can directly help me on problems, because few people means little overlap, and everything is critical all the time which means if I leave everything breaks or stops. At my current job I was spiraling into extremely bad burnout due to this problem, I had to sit down with my bosses and say basically "either I take a long vacation right now or I have to quit, I'm losing my mind". So right now, I'm on a vacation - my first one in over 2 years. Yeah, I don't really get how Polyanna both screens for employers who make her feel like they need her and also complains about companies that don't want you to take vacation. Lots of mismatched expectations up in here.
|
# ? Feb 1, 2018 23:26 |
|
Pollyanna posted:Which is perfectly fair - Im more thinking of this in terms of how truly a company needs me. For example, at one place I interviewed with today, I dug into exactly what the company needed in terms of head count, skill set, etc. Their response was that they dont really need an engineer, though they have wants in terms of skill sets and soft skills - theyre aiming for doubling their user base and everything dependent on their next round of funding and that may pan out, but in the short term i.e. now it didnt seem like they were hurting for a new engineer or anything yet. That made me a little nervous, since there wouldnt be anything clear to point to when asking the question why did we hire her? and makes me feel more superfluous than I am comfortable with. If I get hired, I want to know that I am needed. Stop interviewing at startups. Go apply for a programming job at the MBTA if you want to do something interesting with good job stability. e: https://angel.co/massachusetts-bay-transportation-authority-mbta/jobs/117785-software-engineer Ffs they even ask for Elixir and Phoenix. Weren't you going on a while ago about learning Elixir?
|
# ? Feb 1, 2018 23:52 |
|
Portland Sucks posted:Yeah, I don't really get how Polyanna both screens for employers who make her feel like they need her and also complains about companies that don't want you to take vacation. Lots of mismatched expectations up in here. She's insecure but doesn't want to follow the advice of "get a decent job and stick with it for a few year so you can grow up", thinking she can somehow skip it if she finds the right place or solicits enough advice from SA. Neither of which will happen.
|
# ? Feb 2, 2018 00:13 |
|
Blinkz0rz posted:Stop interviewing at startups. Go apply for a programming job at the MBTA if you want to do something interesting with good job stability. I did interview there. It was close, but they went with someone more senior.
|
# ? Feb 2, 2018 01:04 |
|
In stark contrast to my previous job, where I worked four months and nothing made it out to Production, I already have stuff in Production here, after about three weeks. One of the changes I made was to get rid of the asynchronous behavior when this web app tries to send an email, because the surrounding code always marked the email as having been sent successfully, even when the asynchronous operation ultimately failed. I made this change because, while verifying a different change on Test, I noticed the mail server password was wrong in the configuration. It was wrong on Production, too. I don't know how long, because the logs only go back to the start of this year. So somebody's been clicking "Send Email" for an unknown length of time and been being told it succeeded, without any emails actually having been sent. The reason the operation was asynchronous? "To make interactive, individual sends snappy (at the cost of some reliability)." I took it out and emails send in under a second on Test and Production. Whee!
|
# ? Feb 2, 2018 02:00 |
|
Hughlander posted:If it's large and architect changing like that a hybrid is usually the best approach. A series of commits in a single review that walk through the steps and can be viewed independently or as a whole as up to the person doing the review. Prior to merging this can be a final squash or not depending on local taste, but I'd rather not squash personally. It looks like in both 1 and 2 you're saying that one commit = one code review. That doesn't need to be the case. I can have a code review of 40 commits and do a diff from master to the last commit and get the whole view, or I can dive in and look at individual commits if I get lost, either way though I'm going to comment on the whole thing not on the individual pieces of it since that's what will become master. This is the approach that I like, although it's difficult to pull off at times. It did give me a new appreciation for the magic that could be done with git rebase -i though.
|
# ? Feb 2, 2018 04:09 |
|
How do you guys feel about the following?code:
|
# ? Feb 2, 2018 08:33 |
What's wrong with it?
|
|
# ? Feb 2, 2018 08:40 |
|
Ither posted:How do you guys feel about the following? It's lazy and makes testing a nightmare; The code calling Foo is now tightly coupled with Foo's internal structure. In my current code base we have a horrific data access abstraction that can only be used like that (google "Law of Demeter" for real reasons why it's awful). code:
Do you see what happens when you write Foo.getCollection().add(item)? DO YOU?
|
# ? Feb 2, 2018 08:54 |
|
Yeah at minimum "add" should be a method itself that deals with the logic of inserts internally to the object. If you need to expose the collection in some way (e.g. when you add and a UI has to show the new list) then make it a readonly collection on a getter.code:
code:
code:
|
# ? Feb 2, 2018 09:33 |
|
Shy posted:What's wrong with it? You're modifying a variable using a getter, which at the very least, makes it harder to debug. That's what I was doing when I stumbled upon to that gem.
|
# ? Feb 2, 2018 09:37 |
|
Massive rant was incoming but I cooled down. Short version: we have a Development branch and a feature branch for a pretty big epic so it has been open for a few months now. Yesterday someone finally, after these months, did a merge Development -> featureBranch to keep the second up to date. The part I work on has some big differences as for both the work continued. Because it was a list of so many items, he clicked "Accept Theirs" in the merge window for this entire module, commit this to the featureBranch and pushed to remote. The whole module was broken of course, so I wanted to do a revert but I cannot as git push --force is disabled for everyone by this same developer as "the people in this team are not proficient enough at git to have such a dangerous tool available to them". I lost all day fixing that poo poo working around his bullshit merge. The only good thing was that I was WFH so I was not fired for cursing out my colleagues. Considering I walk away from this place in a week anyway, that would have been fun tho.
|
# ? Feb 2, 2018 10:48 |
|
Pollyanna posted:If I get hired, I want to know that I am needed. So, I am fairly sure you also posted complaints about management desiring too much from jr. dev and planning badly so there are crazy deadlines. If that was not you sorry Good management should hire people preemptively, because people need time to ramp up and become productive. This means that when you are being hired, you are not actually needed, because ideally the management is hiring you because they expect more dev work to be there in the next quarter. This means that there is no crazy pressure to perform right now, but also that there is no need for you, because there is enough time to interview and select for more people. There are exceptions, generally consultants and senior level people are someone who can ramp-up quicker and who you look for when there is crunch, but as long as you are jr. programmer you are not, and should not be, needed. If by needed you meant something more along the lines of "they have long-term plans for me", then that's obviously different, but still, it will be more of a "we have long-term plans for a jr. programmer", not "we have long-term plans for Pollyanna"
|
# ? Feb 2, 2018 10:51 |
|
Keetron posted:Massive rant was incoming but I cooled down. Revert creates a new commit and could be used in that situation. Reset, push force could not, and thats not a bad thing necessarily. Not clear what your problem was.
|
# ? Feb 2, 2018 13:34 |
|
leper khan posted:Revert creates a new commit and could be used in that situation. Reset, push force could not, and thats not a bad thing necessarily. ^^^ Git revert HEAD Git commit amend -c No dad gently caress you Git push origin develop
|
# ? Feb 2, 2018 14:23 |
|
this is totally a healthy and productive career move
|
# ? Feb 2, 2018 15:18 |
|
streetlamp posted:this is totally a healthy and productive career move If not paying means no salary but two digits of equity; unironically maybe correct.
|
# ? Feb 2, 2018 15:42 |
|
Edtech is dumb as hell, just read a book
|
# ? Feb 2, 2018 16:11 |
|
im a developer at a university and yes, its extremely dumb. loving awful poo poo e: guessing off where he is, its basically a full time internship for an adult prolly
|
# ? Feb 2, 2018 17:05 |
|
Doesn't NPR regularly run articles on how edtech has continually shown itself to be a scam and grift vehicle?
|
# ? Feb 2, 2018 18:12 |
|
Rocko Bonaparte posted:I'm looking for some opinions on the scope of commits going into code reviews.
|
# ? Feb 2, 2018 18:25 |
|
Rocko Bonaparte posted:I'm looking for some opinions on the scope of commits going into code reviews. Those both sound pretty unpleasant, but I'd much rather do 2 than 1. More smaller code reviews are both less draining to do, and more likely to generate useful feedback. Something like 2, but using feature branches so that master remains releasable would be my suggestion. If you're seeing architectural changes requested in code reviews, that suggests to me that there need to be feature design reviews ahead of time. Especially on any feature long enough to take a month+ to implement, a 10-30 minute meeting to discuss the planned approach and how it fits into the existing framework before any code is written can save a lot of time.
|
# ? Feb 2, 2018 18:30 |
|
Hughlander posted:^^^ Thank you, you made me laugh.
|
# ? Feb 2, 2018 18:42 |
|
Pollyanna posted:edtech has continually shown itself to be a scam and grift vehicle? No doubt about it. "Here, buy our computer to learn instead of school" doesn't really work at all
|
# ? Feb 2, 2018 18:51 |
|
Vulture Culture posted:The main problem with #1 isn't even that code reviews take a week -- that's a big issue too -- but you're waiting a huge amount of time before people are clued in that a developer has gone off the rails and implemented the absolute wrong thing. Do reviews frequently. Look over things frequently, before someone has wasted a week going off track. Doing dark architectural work against is one approach, I guess, and that's cool if you're okay with having dead, unreferenced code floating around master, but also consider doing more frequent code reviews (even informal ones) on your feature integration branches and then doing a final squash down to your master or develop branch. A secondary cultural thing I am battling in the scenarios I gave is that reviews should be for something being basically ready-to-ship. Let's say I do 50 lines to implement one unit of work towards feature X. I am liable to see "-1 Verify this doesn't do X." TBH I might try doing multiple commits in a review with these guys again. I actually did that awhile ago and they got fixated on the head-most one.
|
# ? Feb 2, 2018 19:12 |
|
Rocko Bonaparte posted:A secondary cultural thing I am battling in the scenarios I gave is that reviews should be for something being basically ready-to-ship.
|
# ? Feb 2, 2018 19:55 |
|
Vulture Culture posted:They should, for merges to master. Merges to master shouldn't merit a code review. Merge master into your feature branch first, with review, and then merge back to master. That way if some jerk (kind, helpful, productive coworker) checks something into master while you're merging, or more likely, during the review, you can still commit the merge changes and easily pick up the new change.
|
# ? Feb 2, 2018 20:23 |
|
rt4 posted:No doubt about it. "Here, buy our computer to learn instead of school" doesn't really work at all Neither do those lazy teachers ami rite ?! But seriously I'm not at all surprised "replace a teacher with a webapp" is a snake oil pit, but I bet making CMSs to facilitate communication and scheduling is probably good, non-scammy business. Also would bet most of them are trash fires, but honest, well-intentioned trash fires.
|
# ? Feb 2, 2018 20:24 |
|
Munkeymon posted:Neither do those lazy teachers ami rite ?! Blackboard is a loving scam made by awful people who hate teachers and professors and if you can find your way into that market you will have a ton of university IT departments that will ignore you because they're trapped by loving Blackboard and cannot hear the screams of the people forced to use it.
|
# ? Feb 2, 2018 23:57 |
|
LLSix posted:Merges to master shouldn't merit a code review. This just seems like willfully talking past each other. The thing that will somehow, some way merged to master should be reviewed and squashed down to 1 commit. Quibbling over the precise ordering of those steps, or proscribing precisely when feature branches are rebased relative to master, or a million other little tangents seems unproductive.
|
# ? Feb 3, 2018 00:01 |
|
Ghost of Reagan Past posted:Blackboard is a loving scam made by awful people who hate teachers and professors and if you can find your way into that market you will have a ton of university IT departments that will ignore you because they're trapped by loving Blackboard and cannot hear the screams of the people forced to use it. Most enterprise software is terrible and supported by a combination of long-term contracts and bureaucratic inertia. Academia must be even worse, since I can't think of anything else that's so exemplified by bureaucratic inertia except the government.
|
# ? Feb 3, 2018 00:07 |
|
Well I'm thinking experiment number 1 is getting reviews of in-progress work that doesn't immediately spun onto master. This might be pissing in the wind, but I'll see how people react to me again sending up a review of a few commits at a time not even set up to hit master.
|
# ? Feb 3, 2018 00:33 |
|
Yeah I kinda refuse to work in edtech. Ive never seen anything good about it. Im sure people will get mad at me for that, but I just dont get a good feeling from it.
|
# ? Feb 3, 2018 01:54 |
|
You're currently unemployed and have been bouncing from job to job where you've never found satisfaction. Maybe don't artificially limit yourself?
|
# ? Feb 3, 2018 02:21 |
|
Blinkz0rz posted:You're currently unemployed and have been bouncing from job to job where you've never found satisfaction. Maybe don't artificially limit yourself? Is Pollyanna unemployed again? What??? I must of missed something. I thought they just had a bad job again.
|
# ? Feb 3, 2018 02:49 |
|
|
# ? May 27, 2024 02:58 |
|
I think Pollyanna has settled for enough poo poo jobs and should try to hold out for something that's tolerable.
|
# ? Feb 3, 2018 02:58 |