Skip to content

Add TLD identifier to premium terms filename and header#2644

Merged
gbrodman merged 1 commit intogoogle:masterfrom
gbrodman:premiumFilename
Jan 24, 2025
Merged

Add TLD identifier to premium terms filename and header#2644
gbrodman merged 1 commit intogoogle:masterfrom
gbrodman:premiumFilename

Conversation

@gbrodman
Copy link
Collaborator

@gbrodman gbrodman commented Jan 17, 2025

https://b.corp.google.com/issues/390053672

This makes it easier to identify what file you're looking at, at a glance


This change is Reviewable

@gbrodman gbrodman requested a review from jianglai January 17, 2025 19:07
Copy link
Contributor

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Are we concerned that users might be expecting a certain format and this will break their processes?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/test/java/google/registry/export/ExportPremiumTermsActionTest.java line 99 at r1 (raw file):

    verify(driveConnection)
        .createOrUpdateFile(
            "CONFIDENTIAL_premium_terms_tld.txt",

If you are using string literals here (and below), do you still need to keep the PREMIUM_TERMS_FILENAME variable?

Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Bruno doesn't have that concern. He says that we could send out an announcement but that an announcement probably isn't even necessary.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai)


core/src/test/java/google/registry/export/ExportPremiumTermsActionTest.java line 99 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

If you are using string literals here (and below), do you still need to keep the PREMIUM_TERMS_FILENAME variable?

what do you mean? I replaced this because of the general guideline to use raw string variables in tests, but we still want the PREMIUM_TERMS_FILENAME_FORMAT constant in the action itself

Copy link
Member

@CydeWeys CydeWeys left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman and @jianglai)


core/src/test/java/google/registry/export/ExportPremiumTermsActionTest.java line 61 at r1 (raw file):

      ImmutableList.of("2048,USD 549", "0,USD 549");
  private static final String EXPECTED_FILE_CONTENT =
      DISCLAIMER_WITH_NEWLINE + TLD_IDENTIFIER_WITH_NEWLINE + "0, 549.00\n" + "2048, 549.00\n";

Best practice in tests is to actually expand the expected strings, rather than duplicating the logic used in the code under test. That also gives the benefit of letting us see in all in one place what the resultant output actually looks like, which can help prompt further refinements.

https://b.corp.google.com/issues/390053672

This makes it easier to identify what file you're looking at, at a
glance
Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @jianglai)


core/src/test/java/google/registry/export/ExportPremiumTermsActionTest.java line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Best practice in tests is to actually expand the expected strings, rather than duplicating the logic used in the code under test. That also gives the benefit of letting us see in all in one place what the resultant output actually looks like, which can help prompt further refinements.

i mean, we were already doing basically what i'm doing here but sure

@jianglai jianglai requested a review from CydeWeys January 22, 2025 13:54
Copy link
Contributor

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/test/java/google/registry/export/ExportPremiumTermsActionTest.java line 99 at r1 (raw file):

Previously, gbrodman wrote…

what do you mean? I replaced this because of the general guideline to use raw string variables in tests, but we still want the PREMIUM_TERMS_FILENAME_FORMAT constant in the action itself

My bad. I had thought that the constant was just used in tests.

Copy link
Member

@CydeWeys CydeWeys left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @jianglai)

@gbrodman gbrodman added this pull request to the merge queue Jan 24, 2025
Merged via the queue into google:master with commit 653e092 Jan 24, 2025
6 checks passed
@gbrodman gbrodman deleted the premiumFilename branch January 24, 2025 20:51
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.

3 participants