Closed Ravisaketi closed 2 years ago
CLA Assistant Lite bot All contributors have signed the CLA ✍️
I have read the CLA Document and I hereby sign the CLA.
Hi @chicco785 @c0c0n3 I have raised a PR for issue #652. can you please check the workflow?
hi @Necravisaketi, it looks like the tests fail for a good reason :-)
UnboundLocalError: local variable 'entity_id' referenced before assignment
Can you try running the tests locally yourself? You should get the same error hopefully...
Hi, @c0c0n3 as per your suggestions I have updated the PR. Please let me know in case any other change is required to merge this PR.
hi @Necravisaketi :-)
can you please check out my PR review to see if what I said makes any sense? Am I misinterpreting what that TODO was about?
hi @Necravisaketi :-)
can you please check out my PR review to see if what I said makes any sense? Am I misinterpreting what that TODO was about?
Hi, @c0c0n3 Yes, you're right. sorry for this mistake from my side and I will update this pr.
Hi @Necravisaketi
sorry for this mistake from my side
nothing to be sorry about. when it comes to software things only get clear as you do them, it's a process. At least, that's how it works for me. I often find myself changing direction as my understanding of the problem space gets better along the way. besides, I'm usually the one making mistakes, so I just wanted to double-check w/ you if I understood the problem right...
@Necravisaketi if you could please give me a shout when you're done and think the PR is ready to merge...
@Necravisaketi if you could please give me a shout when you're done and think the PR is ready to merge...
Hi @c0c0n3, yes, I have already updated this pr. so please review the updated changes.
hi @Necravisaketi I've just had a look at the sql_translator
module and bumped into a few more instances of entity ID and type dict access we could replace w/ the entity_id
and entity_type
functions:
hopefully I caught all the instances, but I can't be 100% sure---spahgetti code :-) if you could please double check...thanks a million!!
Hi, @c0c0n3 I have updated this PR as per your suggestions. so please review the updated changes.
@Necravisaketi I'm merging this PR even if the build failed. In fact, those failures have nothing to do w/ your PR, I've opened separate issues to fix those problems: #666, #674
Proposed changes
Replaced entity with getter. Fix for issued #652
Types of changes
What types of changes does your code introduce to the project? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...