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

Add defs.bzl for //synthesis package #324

Merged
merged 1 commit into from May 5, 2024
Merged

Conversation

abrisco
Copy link
Collaborator

@abrisco abrisco commented May 5, 2024

No description provided.

@abrisco abrisco requested a review from QuantamHD May 5, 2024 16:35
@QuantamHD
Copy link
Collaborator

What's the overall goal with defs.bzl change? Looks harmless overall, but just trying to get better context.

At Google, I don't frequently see the defs.bzl redirection.

@abrisco
Copy link
Collaborator Author

abrisco commented May 5, 2024

What's the overall goal with defs.bzl change? Looks harmless overall, but just trying to get better context.

At Google, I don't frequently see the defs.bzl redirection.

This is to try and reduce the mental overhead of remembering where to load things from or needing to constantly look this up. The two patterns I've found to be most common are <package>/<package>.bzl or <package>/defs.bzl for generally where to load content from. Jumping around from defs.bzl, providers.bzl, build_defs.bzl, etc in the repo currently feels like an unnecessary mental tax and my hope here is to make that better without introducing a breaking change by moving files.

I think it'd be fine to reject this if you don't prefer defs.bzl but it would be nice if module names were predictable across rules_hdl.

@QuantamHD
Copy link
Collaborator

That's totally fine with me. Just wanted to know the motivation.

@QuantamHD QuantamHD merged commit 925fe7c into hdl:main May 5, 2024
4 checks passed
@abrisco abrisco deleted the buildifier branch May 5, 2024 21:07
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