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

Remove unused i18n strings #3932

Closed
wants to merge 3 commits into from
Closed

Remove unused i18n strings #3932

wants to merge 3 commits into from

Conversation

niyid
Copy link
Contributor

@niyid niyid commented Feb 2, 2020

Fixes #3860

To remove the unused key entries in the displayStrings.properties file, a JUnit Test class called bisq.core.util.DisplayPropertiesCheckTest was created which generates a new copy of the stripped file to displayStrings.pruned.properties. It also uses the DisplayPropertiesCheckTest.ignorelist file to provide a list (one per line) of regular expressions matching property keys to ignore.

@niyid niyid requested a review from ripcurlx February 2, 2020 18:29
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK

@niyid Thanks for looking into this, but your code needs to take care not to remove dynamically created keys in code as well. Atm it would remove used keys at well:

  • dao.param.*
  • dao.phase.*
  • dao.bond.bondState.*
  • dao.bond.lockupReason.*
  • dao.bond.bondedRoleType.*
  • dao.assetState.*
  • dao.proposal.type.*
  • dao.tx.type.enum.*
  • *_SHORT
  • NOT remove # comment lines
    have to be excluded as they are used in code.

Also I don't think we should put this into the source directory. Normally this kind of tools are put into the test directory so it is not included in any source build process.

@niyid
Copy link
Contributor Author

niyid commented Feb 4, 2020

  • Converted to JUnit test class.
  • Added ignore-list for dynamically created keys.
  • Comment lines were never removed so no change required for this.

@niyid niyid requested a review from ripcurlx February 4, 2020 16:00
@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 4, 2020

message.state.* has to be excluded as well.

dao.phase.* keys are still removed from the properties file by mistake.

Please remove the pruned properties file afterwards from this PR as I'll run it myself, verify and overwrite the English source file myself.

There is also an issue mentioned in codacy that needs to be fixed. Also please have a look if you have formatted everything correctly before requesting my review.

@niyid
Copy link
Contributor Author

niyid commented Feb 4, 2020

@ripcurlx Please go ahead with your review. Regards.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK

Running testPrune does seem to print correctly UNUSED:... properties, but it creates a displayStrings.pruned.properties file that is identically to displayStrings.properties

@niyid niyid requested a review from ripcurlx February 27, 2020 00:23
@niyid
Copy link
Contributor Author

niyid commented Feb 27, 2020

NACK

Running testPrune does seem to print correctly UNUSED:... properties, but it creates a displayStrings.pruned.properties file that is identically to displayStrings.properties

Apologies. Fixed. Please do not forget to remove the "waiting for author" label. Regards.

@stale
Copy link

stale bot commented Mar 28, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Mar 28, 2020
@stale
Copy link

stale bot commented Apr 4, 2020

This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused i18n strings
2 participants