Skip to content

add config support for macros#12559

Open
ash2shukla wants to merge 5 commits intomainfrom
ash2shukla/fix/move-docs-and-meta-in-config
Open

add config support for macros#12559
ash2shukla wants to merge 5 commits intomainfrom
ash2shukla/fix/move-docs-and-meta-in-config

Conversation

@ash2shukla
Copy link
Contributor

@ash2shukla ash2shukla commented Feb 27, 2026

Resolves #12383
Resolves #9447

Problem

At present we can only declare meta and docs properties on top level whereas they should be supported inside the config as well.

Solution

Create config property with meta and docs and populate them in patch parser with precedence to config.meta and config.docs.

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests, or tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
  • This PR includes type annotations for new and modified functions.

@ash2shukla ash2shukla requested a review from a team as a code owner February 27, 2026 22:24
@cla-bot cla-bot bot added the cla:yes label Feb 27, 2026
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@github-actions
Copy link
Contributor

Additional Artifact Review Required

Changes to artifact directory files requires at least 2 approvals from core team members.

docs = patch.config.get("docs") or patch.docs

# config inherits from HasConfig which is a dict so we need to cast it to Docs
if isinstance(docs, dict):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be handled by defining UnparsedMacroUpdate.config with docs: Docs as well but that breaks the pattern of only needing to inherit from HasColumns.

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.47%. Comparing base (9bb6ebe) to head (70064d2).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12559      +/-   ##
==========================================
- Coverage   91.39%   90.47%   -0.92%     
==========================================
  Files         203      203              
  Lines       25494    25586      +92     
==========================================
- Hits        23301    23150     -151     
- Misses       2193     2436     +243     
Flag Coverage Δ
integration 87.21% <100.00%> (-0.97%) ⬇️
unit 65.36% <50.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 65.36% <50.00%> (-0.06%) ⬇️
Integration Tests 87.21% <100.00%> (-0.97%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ash2shukla
Copy link
Contributor Author

The failing integration tests are due to addition of a top level property config to macros which is missing from the corresponding json schema. This will get fixed once the corresponding changes are checked into fusion and jsonschema is synced.

@ash2shukla ash2shukla self-assigned this Mar 2, 2026
macro.meta = patch.meta
macro.docs = patch.docs

meta = {**(patch.meta or {}), **(patch.config.get("meta") or {})}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the behaviour consistent with how its handled in exposures - see: #11589

@MichelleArk
Copy link
Contributor

This should have no impact to state:modified given config is being newly introduced in this PR, and same_contents for macro resource only considers the macro sql:

class Macro(MacroResource, BaseNode):
@classmethod
def resource_class(cls) -> Type[MacroResource]:
return MacroResource
def same_contents(self, other: Optional["Macro"]) -> bool:
if other is None:
return False
# the only thing that makes one macro different from another with the
# same name/package is its content
return self.macro_sql == other.macro_sql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] False CustomKeyInObjectDeprecation warning for key: macros[].config Nest Macro node meta under config

2 participants