Skip to content

Minor fixes in common.bzl#139

Merged
Szelethus merged 2 commits intoEricsson:mainfrom
nettle:common-fixes
Jan 15, 2026
Merged

Minor fixes in common.bzl#139
Szelethus merged 2 commits intoEricsson:mainfrom
nettle:common-fixes

Conversation

@nettle
Copy link
Copy Markdown
Collaborator

@nettle nettle commented Jan 14, 2026

Why:
Just fix some minor style inconsistency

What:

  • Rename source_attr to SOURCE_ATTR
  • Rename old_bazel_attributes to version_specific_attributes
  • Format load
  • Remove unused code_checker_test rule
  • Move and fix warning function
  • Replace warning with print in aspect

Why:
Just fix some minor style inconsistency

What:
- Rename source_attr to SOURCE_ATTR
- Rename old_bazel_attributes to version_specific_attributes
- Format load
- Remove unused code_checker_test rule
- Remove warning function
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus 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 all of the changes besides the removal of the warning function. What is the design philosophy behind that change? Personally, I think it is a change for the worse.

If we leave that function in, the rest of the patch LGTM.

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Please consider restoring warning().

@Szelethus Szelethus requested a review from furtib January 14, 2026 10:47
@nettle
Copy link
Copy Markdown
Collaborator Author

nettle commented Jan 14, 2026

Hi @furtib and @Szelethus,
Sure, I'll get warning() function back.
There were actually 3 reasons to remove it:

  1. It is used only in one place (src/per_file.bzl)
  2. We do not actually use it, need it, and it should probably replaced by print(). We can discuss.
  3. I guess it does not work as it is, please take a look. You may try to convince me in the opposite.

@nettle
Copy link
Copy Markdown
Collaborator Author

nettle commented Jan 14, 2026

Please consider restoring warning().

Done

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

LGTM

@Szelethus
Copy link
Copy Markdown
Contributor

Hi @furtib and @Szelethus, Sure, I'll get warning() function back. There were actually 3 reasons to remove it:

  1. It is used only in one place (src/per_file.bzl)
  2. We do not actually use it, need it, and it should probably replaced by print(). We can discuss.

The intent of the original patch was to start using is later down the line. Also, we have plenty of unmerged PRs, I'm not sure which ones do actually use it.

  1. I guess it does not work as it is, please take a look. You may try to convince me in the opposite.

Likewise! I guess that is a discussion-worthy topic; the rest of the changes are trivial.

@Szelethus Szelethus merged commit d198b7d into Ericsson:main Jan 15, 2026
10 checks passed
Comment thread src/per_file.bzl
"Unknown file extension for {} defaulting to c++ compile flags".
format(src.short_path)
)
print("Unknown file extension for", src.short_path, "defaulting to C++ compile flags")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Woops, that slipped through. This was meant to be a warning.

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.

2 participants