feat: implement build in source for custom make #417
Conversation
|
Looks good to me, will defer to Daniel's comments |
@mildaniel This is actually something I've gone back and forth on in the past and still unsure about: should we unit test private functions or only public? It seems like it's a topic that has people on both sides, eg.: https://stackoverflow.com/questions/105007/should-i-test-private-methods-or-only-public-ones In this case, I tested The main point I see for not testing private functions is that we'd be testing implementation details rather than behaviour, and also refactoring private functions could mean updating unit tests even if the public behaviour didn't change. |
|
My opinion is that all functions should be unit tested, public or private. I see both arguments, but I think it's a lot easier to cover edge cases on a granular level for each function than it is to try to cover every possible case on a macro level. If we have a module where only 5% of the code is exposed publicly, it's very difficult to write tests to cover all possible scenarios indirectly for the other 95% of code. Very quickly number of cases a higher level test needs to cover grows exponentially and it becomes impossible to thoroughly test the code. Testing coverage will then decrease, and ultimately so will the quality of the tests. This is just my opinion though, so I'm happy to hear what others think. |
| manifest_path, | ||
| osutils=self.os_utils, | ||
| subprocess_make=subprocess_make, | ||
| build_logical_id=build_logical_id, | ||
| working_directory=working_directory, | ||
| ) | ||
|
|
||
| self.actions = [CopySourceAction(source_dir, scratch_dir, excludes=self.EXCLUDED_FILES), make_action] | ||
| self.actions = [] |
There was a problem hiding this comment.
nit: could we still do a list comprehension here? for readability.
There was a problem hiding this comment.
not sure how, given that one item is only added conditionally 🤔
There was a problem hiding this comment.
self.actions = [CopySrc(...), make_action] if .... else [make_action]
There was a problem hiding this comment.
Approving without blocking.
There was a problem hiding this comment.
Merging without this change as I'm not sure if it improves readability and I feel like it wouldn't be able to nicely handle adding more conditional actions in the future.
|
@mildaniel I see your point and I feel like that's one of the reasons I might've tested private functions before. Let me bring it up to the team to see if there's strong opinions. Won't block it on this, but I'll see if there's some consensus. |
torresxb1
force-pushed
the
in-source-custom-make
branch
from
3c92f1a to
c921308
Compare
| manifest_path, | ||
| osutils=self.os_utils, | ||
| subprocess_make=subprocess_make, | ||
| build_logical_id=build_logical_id, | ||
| working_directory=working_directory, | ||
| ) | ||
|
|
||
| self.actions = [CopySourceAction(source_dir, scratch_dir, excludes=self.EXCLUDED_FILES), make_action] | ||
| self.actions = [] |
There was a problem hiding this comment.
Approving without blocking.
torresxb1
force-pushed
the
in-source-custom-make
branch
from
c921308 to
d8279c6
Compare
Issue #, if available:
Description of changes:
Adds the ability to build in source for custom make.
Note: This was branched off another PR: #416. While the parent PR is not merged in, those changes will also show up in this PR's diff. See this PR's actual changes here: torresxb1/aws-lambda-builders@in-source-entry...torresxb1:aws-lambda-builders:in-source-custom-make
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.