Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macros 2.0 #5223

Merged
merged 19 commits into from
Nov 9, 2022
Merged

macros 2.0 #5223

merged 19 commits into from
Nov 9, 2022

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented Sep 7, 2022

Problem

Macros are pain point for a while when it comes to refactoring and code understanding.
I struggled a lot to write documentation for them as interfaces were very vague.

Docs effort:
fixes #5028
fixes #4616

Solution

This is 7.0.2xx(?)/8.0.1xx effort.

  • created better interfaces for macro component based on generics
  • created base types for macro and macro config implementation that do common actions
  • cleaned up the logic of macro processing
    • if macro relies on other variable, and this variable doesn't exist - the processing stops
    • changed logic in GeneratePort macro - previously macro was not releasing socket until the macro is run. Now tracking the list of assigned ports instead.
  • added logging
  • fixed coalesce macro: should fallback value be used when source value is empty string #5028

TODO:

  • add doc on how to add a new macro
  • localization
  • better unit tests
  • agree on coalesce behavior
  • investigate the bug when TemplateCreator can return null
  • investigate how often float and hex types are used
  • add better warnings on low\high validation in port macro

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

@vlada-shubina vlada-shubina force-pushed the refactoring-macros branch 3 times, most recently from a487b4c to 1268485 Compare September 8, 2022 15:00
@vlada-shubina vlada-shubina self-assigned this Sep 13, 2022
@vlada-shubina vlada-shubina added the do-not-merge The PR should not be merged until impediments are resolved. label Sep 23, 2022
@vlada-shubina vlada-shubina marked this pull request as ready for review September 23, 2022 09:09
@vlada-shubina vlada-shubina requested review from a team and baronfel as code owners September 23, 2022 09:09
@vlada-shubina
Copy link
Member Author

@JanKrivanek I would like to kick of the review of this PR, though I'm still working on improving the test coverage.

The areas of review:

  • new macro component interfaces - it would be good to have it pre-ready in case we start to support custom components.
  • updated macro logic and improved parsing of the configuration with error handling.
  • documentation

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Overall looks good. I like the extraction of the common settings/validation code!

I havent look into actual macros in depth (as I didn't expect much changes, other than extracting config and getting rid of repetitive validation ocde) - I will need to do a second pass on those.

@vlada-shubina vlada-shubina force-pushed the refactoring-macros branch 2 times, most recently from f33940e to a06754a Compare October 4, 2022 15:55
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I like the changes!

I have only 1 minor naming suggestion, other than that it looks ready to go

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Coalesce functionality looks good. I have just some notes to doc

docs/Available-Symbols-Generators.md Outdated Show resolved Hide resolved
docs/Available-Symbols-Generators.md Outdated Show resolved Hide resolved
@vlada-shubina
Copy link
Member Author

Open tasks:

  • agree on coalesce behavior
  • investigate bug when TemplateCreator can return null
  • investigate how often float and hex types are used

@vlada-shubina vlada-shubina force-pushed the refactoring-macros branch 2 times, most recently from bd6a24b to d6d7faa Compare November 4, 2022 15:43
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I agree with reverting the changes in the coallesce macro.
Overall looks good!

@vlada-shubina vlada-shubina removed the do-not-merge The PR should not be merged until impediments are resolved. label Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants