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

Improve strict module layout #769

Merged
merged 4 commits into from Apr 10, 2020

Conversation

sasa1977
Copy link
Contributor

@sasa1977 sasa1977 commented Apr 6, 2020

This PR brings in a few minor changes/additions to the StrictModuleLayout check. These additions are motivated by the real usage in the project of my client (which is where the original implementation of this check was done).

Note that these changes weren't discussed in any issue so far. However, to better understand the issues, I experimented with a fix, and so I ended up with the solution here, and then I figured I may as well submit it :-) That said, I'm not insisting on these changes, and so if you have some other ideas feel free to propose them.

Anyway, here are the changes:

  1. Callback macros and callback functions are bundled under the same key callback_impl. In other words, with this change we can't enforce that e.g. callback macros must preceed callback functions. The reason is because a single module could implement multiple behaviours, and then such order becomes counterproductive, because we typically want to group callbacks by the module (e.g. first all callbacks of Foo, then all callbacks of Bar).
  2. Added the :ignore option which allows us to ignore some module parts (i.e. allow some parts to appear anywhere in the module). For my client's projects, the concrete example would be private macros.

@rrrene
Copy link
Owner

rrrene commented Apr 6, 2020

I am very much in favor of this.

How do we ensure some kind of backwards compatibility? I do understand that there can't 100% backwards compatibility in this case. But could do something that makes this change less "breaking"?

Would it make sense to include a warning for existing configs that include :callback_fun? Would it be okay to rewrite existing :callback_fun values to :callback_impl + print a warning about it?

@sasa1977
Copy link
Contributor Author

sasa1977 commented Apr 6, 2020

Would it be okay to rewrite existing :callback_fun values to :callback_impl + print a warning about it?

Yeah, that's a good suggestion! I'll add it.

@rrrene rrrene merged commit 750dea0 into rrrene:master Apr 10, 2020
@rrrene
Copy link
Owner

rrrene commented Apr 10, 2020

@sasa1977 👍 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants