Rename NFT to NFToken for lsfDisallowIncomingNFTokenOffer flags #4442

kennyzlei · 2023-03-01T19:09:58Z

High Level Overview of Change

Rename lsfDisallowIncomingNFTOffer to lsfDisallowIncomingNFTokenOffer
Rename asfDisallowIncomingNFTOffer to asfDisallowIncomingNFTokenOffer
Update spacing on TXFlags.h after rename

Context of Change

NFToken is the more standard naming convention in the codebase compared to NFT

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

thejohnfreeman

Not generally an advocate of rename-only changesets, but I'm fine approving work that has already been done.

thejohnfreeman · 2023-03-01T20:24:58Z

src/ripple/protocol/TxFlags.h

@@ -147,15 +147,15 @@ constexpr std::uint32_t const tfNFTokenMintMask =
    ~(tfUniversal | tfBurnable | tfOnlyXRP | tfTransferable);

 // NFTokenCreateOffer flags:
-constexpr std::uint32_t const tfSellNFToken            = 0x00000001;
+constexpr std::uint32_t const tfSellNFToken                = 0x00000001;
 constexpr std::uint32_t const tfNFTokenCreateOfferMask =


It seems the equals signs in this file are aligned with the one on this line because it has the longest prefix. Two things:

After changing "NFT" to "NFToken", the new longest prefix is on line 85. The equals sign on that line should be one space apart from that prefix. In this commit, it is four spaces apart.

The equals sign on this line, 151, should be aligned with the others in the file.

happy to go with that spacing guidance, i had assumed it was a nearest every 4 spaces approach since the last one happened to be % 4 in a clean way

@thejohnfreeman for point 2. I think I have seen a different approach in other lines that fold so I thought it was more consistent to not align when they fold, see lines https://github.com/XRPLF/rippled/blob/develop/src/ripple/protocol/TxFlags.h#L67 for example

Out of curiosity, has it ever been considered to have formatting rules controlled by a linting tool automatically?

The file as it looked before clang-format hit it in the Great Reformat appears to have every equals sign aligned, even on lines that wrap. They are all in column 44, and they are all 4 spaces after the longest prefix. Both of these rules match the alignment you have, @kennyzlei, except for the wrapped lines that you are not touching. Let's keep that alignment and just add a comment at the start, before or after the // clang-format off line, explaining this rule. Optionally, you can restore the wrapped lines to conform to the rule.

@justinr1234 We do have that. It's controlled by clang-format. Style configuration in .clang-format. Enforcement in .github/workflows/clang-format.yml. We disable it for sections of special formatting like this. When deciding to incorporate it, the majority wanted to keep the special formatting.

@thejohnfreeman added a comment to describe the special formatting

intelliot · 2023-03-01T21:31:03Z

Not generally an advocate of rename-only changesets, but I'm fine approving work that has already been done.

I don't understand this sentence. Naming is important, and it seems clear that this rename follows the principle of least surprise. If we agree that the rename is worth doing, then it is not a problem to do it in a rename-only changeset.

thejohnfreeman · 2023-03-02T19:39:39Z

@kennyzlei Please, in the future, no more force-pushes. Not sure what is going on with review assignments either. Generally, assignments are left up even after a review is submitted.

`lsfDisallowIncomingNFTOffer` to `lsfDisallowIncomingNFTokenOffer` `asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

`asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

kennyzlei · 2023-03-02T20:52:34Z

😞 sorry I had to force push again here as my initial commits did not come with pgp signature, i thought i had auto-signed set up

intelliot · 2023-03-02T20:57:15Z

No worries; I think the signature changes when the commits get squashed anyway. This looks fine to me: I'll go ahead and merge; if we find any problems, we can fix with a follow-up PR.

* upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432) Update package building scripts and images to use Conan (XRPLF#4435) Disable duplicate detector: (XRPLF#4438)

* upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432)

…ctionality * upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432) Update package building scripts and images to use Conan (XRPLF#4435) Disable duplicate detector: (XRPLF#4438)

…tpage * upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432) Update package building scripts and images to use Conan (XRPLF#4435) Disable duplicate detector: (XRPLF#4438)

* Follow-up to XRPLF#4336 * NFToken is the naming convention in the codebase (rather than NFT) * Rename `lsfDisallowIncomingNFTOffer` to `lsfDisallowIncomingNFTokenOffer` * Rename `asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer`

`lsfDisallowIncomingNFTOffer` to `lsfDisallowIncomingNFTokenOffer` `asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

`asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

kennyzlei requested review from RichardAH, seelabs and thejohnfreeman March 1, 2023 20:17

thejohnfreeman requested changes Mar 1, 2023

View reviewed changes

kennyzlei assigned thejohnfreeman Mar 1, 2023

intelliot added this to the 1.10.0 milestone Mar 1, 2023

intelliot requested review from nbougalis, ckniffen and justinr1234 March 1, 2023 21:32

ckniffen mentioned this pull request Mar 1, 2023

fix: rename DisallowIncoming flag for NFTs ripple/explorer#615

Merged

1 task

ckniffen approved these changes Mar 1, 2023

View reviewed changes

justinr1234 approved these changes Mar 2, 2023

View reviewed changes

intelliot requested a review from thejohnfreeman March 2, 2023 17:23

intelliot assigned kennyzlei and unassigned thejohnfreeman Mar 2, 2023

intelliot removed the request for review from thejohnfreeman March 2, 2023 17:24

kennyzlei force-pushed the rename-lsfDisallowIncomingNFTOffer branch 2 times, most recently from 12bf9b1 to e90a481 Compare March 2, 2023 19:23

kennyzlei removed their assignment Mar 2, 2023

kennyzlei requested a review from thejohnfreeman March 2, 2023 19:25

kennyzlei assigned thejohnfreeman Mar 2, 2023

thejohnfreeman approved these changes Mar 2, 2023

View reviewed changes

kennyzlei unassigned thejohnfreeman Mar 2, 2023

kennyzlei added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 2, 2023

ckniffen mentioned this pull request Mar 2, 2023

fix: rename DisallowIncoming flag for NFTs XRPLF/xrpl.js#2235

Merged

1 task

ckniffen added a commit to XRPLF/xrpl-py that referenced this pull request Mar 2, 2023

rename DisallowIncoming flag for NFTs

962deea

`asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

ckniffen mentioned this pull request Mar 2, 2023

rename DisallowIncoming flag for NFTs XRPLF/xrpl-py#533

Merged

1 task

kennyzlei added 3 commits March 2, 2023 12:50

rename NFT to NFToken for disallowIncomingNFToken flags

9d86c7b

spacing

03b03b4

comment on special formatting, clang-format off

0585db3

kennyzlei force-pushed the rename-lsfDisallowIncomingNFTOffer branch from e90a481 to 0585db3 Compare March 2, 2023 20:51

intelliot merged commit ecd49e1 into XRPLF:develop Mar 2, 2023

intelliot mentioned this pull request Mar 2, 2023

Set version to 1.10.0-rc4 #4444

Merged

1 task

kennyzlei deleted the rename-lsfDisallowIncomingNFTOffer branch March 3, 2023 22:33

kennyzlei removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 3, 2023

intelliot mentioned this pull request Mar 6, 2023

featureDisallowIncoming: Opt-out of incoming Checks, PayChans, NFTokenOffers and Trustlines #4336

Merged

7 tasks

ckniffen added a commit to XRPLF/xrpl-py that referenced this pull request Mar 9, 2023

rename DisallowIncoming flag for NFTs (#533)

9818695

`asfDisallowIncomingNFTOffer` to `asfDisallowIncomingNFTokenOffer` Related to XRPLF/rippled#4442

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename NFT to NFToken for lsfDisallowIncomingNFTokenOffer flags #4442

Rename NFT to NFToken for lsfDisallowIncomingNFTokenOffer flags #4442

kennyzlei commented Mar 1, 2023

thejohnfreeman left a comment

thejohnfreeman Mar 1, 2023

kennyzlei Mar 1, 2023

kennyzlei Mar 1, 2023

justinr1234 Mar 2, 2023

thejohnfreeman Mar 2, 2023

kennyzlei Mar 2, 2023

intelliot commented Mar 1, 2023

thejohnfreeman commented Mar 2, 2023

kennyzlei commented Mar 2, 2023

intelliot commented Mar 2, 2023

Rename NFT to NFToken for lsfDisallowIncomingNFTokenOffer flags #4442

Rename NFT to NFToken for lsfDisallowIncomingNFTokenOffer flags #4442

Conversation

kennyzlei commented Mar 1, 2023

High Level Overview of Change

Context of Change

Type of Change

thejohnfreeman left a comment

Choose a reason for hiding this comment

thejohnfreeman Mar 1, 2023

Choose a reason for hiding this comment

kennyzlei Mar 1, 2023

Choose a reason for hiding this comment

kennyzlei Mar 1, 2023

Choose a reason for hiding this comment

justinr1234 Mar 2, 2023

Choose a reason for hiding this comment

thejohnfreeman Mar 2, 2023

Choose a reason for hiding this comment

kennyzlei Mar 2, 2023

Choose a reason for hiding this comment

intelliot commented Mar 1, 2023

thejohnfreeman commented Mar 2, 2023

kennyzlei commented Mar 2, 2023

intelliot commented Mar 2, 2023