Skip to content

Conversation

@AkshayKumarSahu
Copy link
Contributor

Description:

Related issue(s):

Fixes #1544

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Copilot AI review requested due to automatic review settings January 26, 2026 10:58
@github-actions
Copy link

[commit-verification-bot]
Hi, this is VerificationBot.
Your pull request cannot be merged as it has unverified commits.
View your commit verification status: Commits Tab.

To achieve verified status, please read:

Remember, you require a GPG key and each commit must be signed with:
git commit -S -s -m "Your message here"

Thank you for contributing!

From the Hiero Python SDK Team

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

Walkthrough

Applied Black code formatter to standardize style in a test file and added corresponding changelog entry documenting the formatting update.

Changes

Cohort / File(s) Summary
Changelog Entry
CHANGELOG.md
Added entry for Black formatting of tests/unit/nft_id_test.py (#1544).
Test File Formatting
tests/unit/nft_id_test.py
Applied Black formatting: reflowed multiline constructions, added trailing commas, reorganized assertions for readability, adjusted whitespace and empty lines. No functional or logic changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses the core objective from #1544 by running Black formatting on tests/unit/nft_id_test.py; however, it fails to meet multiple acceptance criteria including signed commits (DCO/GPG), proper changelog entry placement, and CI/workflow checks. Ensure commits are signed with DCO and GPG (git commit -S -s), move changelog entry to the appropriate section (Tests or Changed), and verify all CI/workflow checks pass before merging.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: applying Black formatting to tests/unit/nft_id_test.py, which is directly reflected in the code changes.
Description check ✅ Passed The description references the related issue #1544 and indicates the PR is meant to apply Black formatting, which aligns with the actual code changes shown in the summary.
Out of Scope Changes check ✅ Passed All changes are in-scope: CHANGELOG.md updated for the formatting task and tests/unit/nft_id_test.py contains only Black formatting adjustments with no behavioral modifications.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Comment on lines 36 to +53
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid1 = TokenId(shard=0, realm=1, num="A")
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid = TokenId(shard=0, realm="b", num=1)
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid = TokenId(shard='c', realm=1, num=1)
nftid_failed_constructor_tokenid = TokenId(shard="c", realm=1, num=1)
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=None, serial_number=1234)
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=1234, serial_number=1234)
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number="asdfasdfasdf")
nftid_failed_constructor = NftId(
token_id=TokenId(shard=0, realm=1, num=0), serial_number="asdfasdfasdf"
)
with pytest.raises(ValueError):
nftid_failed_constructor = NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number=-1234)
nftid_failed_constructor = NftId(
token_id=TokenId(shard=0, realm=1, num=0), serial_number=-1234
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove unused local assignments in failing-constructor tests.

These locals are never used; they can be removed (or assigned to _) to keep tests clean and avoid Ruff F841.

✅ Suggested cleanup
-    with pytest.raises(TypeError):
-        nftid_failed_constructor_tokenid1 = TokenId(shard=0, realm=1, num="A")
+    with pytest.raises(TypeError):
+        TokenId(shard=0, realm=1, num="A")

-    with pytest.raises(TypeError):
-        nftid_failed_constructor_tokenid = TokenId(shard="c", realm=1, num=1)
+    with pytest.raises(TypeError):
+        TokenId(shard="c", realm=1, num=1)

-    with pytest.raises(TypeError):
-        nftid_failed_constructor = NftId(
-            token_id=TokenId(shard=0, realm=1, num=0), serial_number="asdfasdfasdf"
-        )
+    with pytest.raises(TypeError):
+        NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number="asdfasdfasdf")

-    with pytest.raises(ValueError):
-        nftid_failed_constructor = NftId(
-            token_id=TokenId(shard=0, realm=1, num=0), serial_number=-1234
-        )
+    with pytest.raises(ValueError):
+        NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number=-1234)
🧰 Tools
🪛 Ruff (0.14.13)

37-37: Local variable nftid_failed_constructor_tokenid1 is assigned to but never used

Remove assignment to unused variable nftid_failed_constructor_tokenid1

(F841)


41-41: Local variable nftid_failed_constructor_tokenid is assigned to but never used

Remove assignment to unused variable nftid_failed_constructor_tokenid

(F841)


50-50: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception

(PT011)


51-51: Local variable nftid_failed_constructor is assigned to but never used

Remove assignment to unused variable nftid_failed_constructor

(F841)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Formats tests/unit/nft_id_test.py with Black to standardize unit test style (Fixes #1544).

Changes:

  • Apply Black-compatible formatting to tests/unit/nft_id_test.py (line wrapping, spacing, trailing commas).
  • Add an Unreleased changelog entry noting the formatting change.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File Description
tests/unit/nft_id_test.py Black-style formatting updates for consistent unit test formatting.
CHANGELOG.md Adds an Unreleased entry documenting the test formatting change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CHANGELOG.md Outdated
- Formatted client_test.py using Black.

### Added
- Formatted black tests/unit/nft_id_test.py
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Changelog entry is inconsistent with nearby formatting entries: it’s under ### Added even though it’s a test formatting/style change (there is already a ### Tests section above), it doesn’t follow the existing wording pattern (e.g., "Formatted ... using Black."), and it’s missing an issue/PR reference in parentheses per docs/sdk_developers/changelog_entry.md (step 5). Consider moving this bullet under ### Tests, rewording to match the surrounding entries, and adding (#1544) (or the PR number).

Copilot uses AI. Check for mistakes.
)

#don't need to test protobuf cause its final and type checked
# don't need to test protobuf cause its final and type checked
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

This comment has grammar issues ("cause"/"its"). Since this line was touched in this PR, consider updating it to something like "because it's final and type-checked" for clarity.

Suggested change
# don't need to test protobuf cause its final and type checked
# don't need to test protobuf because it's final and type-checked

Copilot uses AI. Check for mistakes.
nftid_failed_constructor_tokenid = TokenId(shard=0, realm="b", num=1)
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid = TokenId(shard='c', realm=1, num=1)
nftid_failed_constructor_tokenid = TokenId(shard="c", realm=1, num=1)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Variable nftid_failed_constructor_tokenid is not used.

Copilot uses AI. Check for mistakes.
)
with pytest.raises(ValueError):
nftid_failed_constructor = NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number=-1234)
nftid_failed_constructor = NftId(
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Variable nftid_failed_constructor is not used.

Copilot uses AI. Check for mistakes.
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid1 = TokenId(shard=0, realm=1, num="A")
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid = TokenId(shard=0, realm="b", num=1)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

This assignment to 'nftid_failed_constructor_tokenid' is unnecessary as it is redefined before this value is used.

Copilot uses AI. Check for mistakes.
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=None, serial_number=1234)
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=1234, serial_number=1234)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

This assignment to 'nftid_failed_constructor' is unnecessary as it is redefined before this value is used.

Copilot uses AI. Check for mistakes.
nftid_failed_constructor = NftId(token_id=1234, serial_number=1234)
with pytest.raises(TypeError):
nftid_failed_constructor = NftId(token_id=TokenId(shard=0, realm=1, num=0), serial_number="asdfasdfasdf")
nftid_failed_constructor = NftId(
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

This assignment to 'nftid_failed_constructor' is unnecessary as it is redefined before this value is used.

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +9


Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that your commits are currently showing as 'Unverified.' For this repository, we require commits to be signed. You can follow this GitHub guide on Commit Signature Verification to set that up. You may need to rebase or amend your commits with a signature.

Comment on lines +13 to 37
nftid_constructor_test = NftId(
token_id=nftid_constructor_tokenid, serial_number=1234
)

assert str(nftid_constructor_test) == "0.1.2/1234"
assert repr(nftid_constructor_test) == "NftId(token_id=TokenId(shard=0, realm=1, num=2, checksum=None), serial_number=1234)"
assert (
repr(nftid_constructor_test)
== "NftId(token_id=TokenId(shard=0, realm=1, num=2, checksum=None), serial_number=1234)"
)
assert nftid_constructor_test._to_proto().__eq__(
basic_types_pb2.NftID(
token_ID=basic_types_pb2.TokenID(shardNum=0, realmNum=1, tokenNum=2),
serial_number=1234)
serial_number=1234,
)
)
assert NftId._from_proto(
nft_id_proto=basic_types_pb2.NftID(
token_ID=basic_types_pb2.TokenID(shardNum=0, realmNum=1, tokenNum=2),
serial_number=1234
serial_number=1234,
)
).__eq__(nftid_constructor_test)

#return false
# return false
with pytest.raises(TypeError):
nftid_failed_constructor_tokenid1 = TokenId(shard=0, realm=1, num="A")

This comment was marked as resolved.

@Akshat8510

This comment was marked as resolved.

@AkshayKumarSahu
Copy link
Contributor Author

@Akshat8510 hi thanks for your feedback but this i have done via black command only i have nothing manual.

@Akshat8510
Copy link
Contributor

Okay, Once you've also signed the commit and moved the Changelog entry to the 'Tests' or 'Changed' section, I'll be ready to approve!

Copy link
Contributor

@aceppaluni aceppaluni left a comment

Choose a reason for hiding this comment

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

@AkshayKumarSahu Thank you for your contribution!

I have linked our Signing.md file for assistance with verifying commits.

If you have questions or need assistance let us know. We'd be happy to help!

@manishdait
Copy link
Contributor

Hi @AkshayKumarSahu,
One of the commits a9338b3 was marked unverified cause because the committer email might not associated with a GitHub account.
This can be fixed by amending the affected commits using a verified email:

git config --global user.email "your_verified_email"
git rebase -i HEAD~2
# mark the affected commits as `edit`

git commit --amend --no-edit
git rebase --continue
git push --force-with-lease

@github-actions
Copy link

Hello, this is the OfficeHourBot.

This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC).

This session provides an opportunity to ask questions regarding this Pull Request.

Details:

Disclaimer: This is an automated reminder. Please verify the schedule here for any changes.

From,
The Python SDK Team

@aceppaluni
Copy link
Contributor

@AkshayKumarSahu

If you need any assistance please let us know, our team is happy to help!

@github-actions
Copy link

Hi, this is MergeConflictBot.
Your pull request cannot be merged because it contains merge conflicts.

Please resolve these conflicts locally and push the changes.

To assist you, please read:

Thank you for contributing!

Copy link
Contributor

@Akshat8510 Akshat8510 left a comment

Choose a reason for hiding this comment

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

Hi @AkshayKumarSahu , I noticed your first commit (a9338b3) is Unverified. Please amend that commit to include a signature and force-push.

@Akshat8510
Copy link
Contributor

You can find instructions on how to set this up here : https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/signing.md

CHANGELOG.md Outdated
- Formatted client_test.py using Black.

### Added
- Formatted black tests/unit/nft_id_test.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move your entry from ### Added to ### Tests (create the section if needed). Update the wording to: “Formatted tests/unit/nft_id_test.py using Black.” and append the PR number at the end, e.g. (# 1599).

@AkshayKumarSahu
Copy link
Contributor Author

for some reason i am unable to verify previous commit
I am closing this pr
Raising a new one
@Akshat8510 @aceppaluni @manishdait thanks for the help

@Akshat8510
Copy link
Contributor

If you feel more comfortable starting fresh with a new PR where every commit is signed from the start, that is perfectly fine! But if you'd like to try fixing this one instead, let us know and we can walk you through the rebase steps. Either way works for us!

Signed-off-by: AkshayKumarSahu <your_verified_email>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1


### Tests

- Format `tests/unit/nft_id_test.py with` with Black.(#1544)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix typo in changelog entry: misplaced backtick.

The backtick is misplaced, creating a malformed code span. The entry currently reads `tests/unit/nft_id_test.py with` instead of `tests/unit/nft_id_test.py`.

🔧 Proposed fix
-- Format `tests/unit/nft_id_test.py with` with Black.(`#1544`)
+- Format `tests/unit/nft_id_test.py` with Black (`#1544`)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Format `tests/unit/nft_id_test.py with` with Black.(#1544)
- Format `tests/unit/nft_id_test.py` with Black (`#1544`)

@Akshat8510
Copy link
Contributor

You’ve actually done excellent work here, Akshay! Since everything is already squashed into a single commit, there’s no need to start a new PR from scratch.

@Akshat8510
Copy link
Contributor

To finish it in this PR, just reopen it, run git commit --amend -S -s, and then git push --force. That will fix both the verification and the DCO at the same time.

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.

[Good First Issue]: format black tests/unit/nft_id_test.py

4 participants