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

Configure layer internal tag #1318

Merged

Conversation

brightbyte
Copy link

Make @internal tag configurable

Some projects, like MediaWiki, use the @internal tag with a different
semantics than the one imposed by deptrac. In the case of MediaWiki, the
intended semantics is "not for use by extensions", i.e. "internal to the
core repo".

To work around this issue, allow the config file to specify which tag
deptrac will interpret as "layer-internal". The @deptrac-internal tag
will continue to always work.

Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

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

Tests need fixing, but otherwise LGTM.

@brightbyte
Copy link
Author

Tests need fixing, but otherwise LGTM.

Thank you for the quick response! I'll sort out the tests later today, sorry for the mess.

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

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

In most of your tests, you are passing an empty array as a parameter. This is no longer valid as you have specified in the __constructor PHPDoc that the array always has the key present. I think the PHPStan static analyser should complain about it too.

Otherwise, it is really close. Just small things here and there.

@brightbyte

This comment was marked as resolved.

Collect all @tags from doc blocks and exposes them on
ClassLikeReference and FunctionReference objects.
This collector allows to define a layer based on doc-block tags on
classes and functions. The definition can include a regular expression
that the tag value must match.
Some projects, like MediaWiki, use the @internal tag with a different
semantics than the one imposed by deptrac. In the case of MediaWiki, the
intended semantics is "not for use by extensions", i.e. "internal to the
core repo".

To work around this issue, allow the config file to specify which tag
deptrac will interpret as "layer-internal". The @deptrac-internal tag
will continue to always work.

NOTE: This is a breaking change: the "@internal" tag will will now be
ignored unless explicitly configured to be used.
- fix documentation
- use explicit null check
/** @var array<string, EmitterType> */
private array $analyser = [];
/** @var array<string, array<string>> */
private array $skipViolations = [];
/** @var array<string> */
private array $excludeFiles = [];

public function internalTag(?string $tag): self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it a standalone function and not a parameter to public function analysers? It is incongruent with how it is specified in the yaml config.

Copy link
Author

Choose a reason for hiding this comment

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

The analysers() function is already a bit odd... it sets the "types" key in the "analyser" section, not the entire section. But sure, we could have one parameter for each key in that section.

Keep in mind we will add another one soon, for skipDeprecated. I generally dislike long lists of parameters. But since PHP 8 has named params, perhaps I should learn to love them.

Alternatively, there could be a separate AnalyzerConfig object, and internalTag() and skipDeprecated() could me methods on that object...

Copy link
Collaborator

Choose a reason for hiding this comment

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

AnalyzerConfig seems like the logical consistent way to go to me.

Copy link
Author

Choose a reason for hiding this comment

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

done

Analyser is defined analogous to Layer.

This adds a phpunit test for the relevant parts of DeptracConfig.
Also: in our own deptrac config, set internal_tag to "@internal",
since that's the tag that is used in a couple of places.
@gennadigennadigennadi gennadigennadigennadi merged commit 35aa912 into qossmic:main Jan 19, 2024
17 of 18 checks passed
@gennadigennadigennadi
Copy link
Member

@brightbyte thank you very much for your contributing, I looking forward to see more of your work land in deptrac.

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

Successfully merging this pull request may close these issues.

None yet

3 participants