consolidate flags #6788
consolidate flags #6788
Conversation
|
|
||
| # set the default flags | ||
| for key, value in flag_defaults.items(): | ||
| object.__setattr__(self, key, value) |
There was a problem hiding this comment.
Not directly related to this changeset, so feel free to ignore this comment.
I think this flags code would be simpler and more readable if we added _get_flag() and _set_flag() functions which handled the casing concerns and the use of the setattr/getattr functions.
There was a problem hiding this comment.
Yes, I was chatting with @MichelleArk about similar thing also. @iknox-fa might have something around it, but we are gonna defer that to work later on instead of in this PR
There was a problem hiding this comment.
I didn't do that the first time around because I wanted to keep things as immutable as python allows so adding even a "private" method to do that seemed like a recipe for mutated flags in the codebase in a few months.
Maybe as a compromise for readability we just make nested get and set functions in init? That way no-one uses them unless they're directly editing the flags? We can discuss it further but given how much of a tangled mess the flags are currently I think we're going to get a lot of mileage out of having reasonable assurance of immutability and I don't want to jeopardize that.
ChenyuLInx
force-pushed
the
cl/consolidate_flags
branch
from
5498310 to
2b7afc8
Compare
core/dbt/cli/flags.py
Outdated
| f"Duplicate flag names found in click command: {param_name}" | ||
| ) | ||
| else: | ||
| if param_name not in params_assigned_from_default: |
There was a problem hiding this comment.
I'm not sure this is working as expected. params_assigned_from_default isn't an accurate representation of which params have not been set by the user within assign_params as the assign_params method itself is what populates params_assigned_from_default recursively.
check out the breaking test: tests/functional/simple_seed/test_seed.py::TestBasicSeedTests::test_simple_seed_full_refresh_flag, which sets --full-refresh by the user but is not reflected in Flags.
There was a problem hiding this comment.
fixed with new logic, slightly cleaner, I would like to defer the duplicate param issue to a new issue, maybe after merge. @iknox-fa thoughts?
ChenyuLInx
force-pushed
the
cl/consolidate_flags
branch
from
9095034 to
003ad78
Compare
MichelleArk
force-pushed
the
cl/consolidate_flags
branch
2 times, most recently
from
9dcea2f to
09c1171
Compare
|
|
||
| class TestDbtRunner: | ||
| @pytest.fixture | ||
| def dbt(self) -> dbtRunner: |
There was a problem hiding this comment.
This feels like gonna introduce some overhead of setting up the project to unittests?
ChenyuLInx
force-pushed
the
cl/consolidate_flags
branch
from
abcb34c to
9b9c908
Compare
resolves #6701
This moves the remaining default to cli/flags.py
We still have a few being accessed by legacy logger that remains in flags module.
TODO
project_dirandprofiles_dirat link not carried over and cause some additional tests fail.