refactor: simplify node npm workflow conditionals #458
Conversation
| source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm | ||
| ) | ||
|
|
||
| def actions_without_bundler(self, source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm): |
There was a problem hiding this comment.
this was originally meant for when the workflow would have actions_with_bundler (for esbuild) and actions_without_bundler. But the bundler actions were split into a separate workflow (esbuild), so there's not much use for having actions_without_bundler in a separate function. So I just moved the logic into the workflow constructor.
| LOG.info( | ||
| "download_dependencies is False and dependencies_dir is None. Copying the source files into the " | ||
| "artifacts directory. " | ||
| ) |
There was a problem hiding this comment.
I thought this message was a bit weird and not totally accurate, so I removed it. The message could be printed even if dependencies_dir is not None (if combine_dependencies is False) . Let me know if you think I should bring this message back.
There was a problem hiding this comment.
Note that the conditions that lead to this message being output is not a valid SAM CLI use case anyway (shouldn't be very common):
https://github.com/aws/aws-lambda-builders/pull/272/files#r701280071
| return | ||
|
|
||
| # if we downloaded dependencies, update dependencies_dir | ||
| if self.download_dependencies: |
There was a problem hiding this comment.
are we branching on the same condition twice?
There was a problem hiding this comment.
One other general comment is we have more than a few branches, we should go down a handler route where we mutate on a a given object all the way through.
eg: self.actions is the one we mutate in this workflow, something like below would be useful to grok through it.
for action_mapper in action_mappers:
action_mapper.apply(*args, **kwargs)There was a problem hiding this comment.
are we branching on the same condition twice?
The first time it doesn't depend on self.dependencies_dir. The second time we can assume we have self.dependencies_dir (since we checked it before).
There was a problem hiding this comment.
this part in the middle of those:
There was a problem hiding this comment.
The first time it doesn't care about, or depend on, self.dependencies_dir. The second time, since we have checked dependencies_dir, we can assume we have it and we don't have to check for it again. Otherwise it would require some conditional nesting or repetitive conditions, which is what I'm trying to refactor out here 😅
There was a problem hiding this comment.
to me this is not intuitive and requires a further refactor.
and is also repeated.There was a problem hiding this comment.
^ the above is also due to something similar. One is performed when there's no dependencies dir and one is performed when there is.
To me the refactor is easier to read than before, but let me know if you disagree. Also open to suggestions on how to further refactor.
|
Note: I still believe this requires an overall refactor, there are repeated statements and branching logic that looks identical even though the state that they operate on could be different. |
|
Created an internal ticket to suggest further, bigger, refactoring improvements. |
3 participants
Issue #, if available:
Description of changes:
Currently there are a few nested conditions in the workflow. This PR refactors to remove some nesting and try to simplify the logic.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.