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

Rename user-facing instances of 'personal token' to 'time token' #8322

Merged
merged 2 commits into from
Feb 2, 2021

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Jan 28, 2021

Rename "Personal Tokens" to "Time Tokens". To simplify the renaming process, variables, classes, and contracts continue to use the old name, but user-facing text uses the new name. Personal tokens and Time tokens are the same thing, so you will likely see those two phrases used interchangably throughout the codebase.

@thelostone-mc One of the changes is to a migration file 0150_auto_20200904_1505.py—should this file be changed, or should it be left alone?

Copy link
Contributor

@chibie chibie left a comment

Choose a reason for hiding this comment

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

why not replace occurrences of ptoken / ptokens with ttoken / ttokens ? 👀

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@mds1 I'd say let's not update the 0150_auto_20200904_1505!
Instead create a new migration file
It makes it cleaner and less messier on stable as 0150 has already been executed in live

@mds1 mds1 force-pushed the ptoken-to-ttoken branch from a4cb8b0 to 3337e30 Compare February 1, 2021 12:54
@mds1
Copy link
Contributor Author

mds1 commented Feb 1, 2021

why not replace occurrences of ptoken / ptokens with ttoken / ttokens ? 👀

@chibie Just because it's a lot simpler not to! If we rename the variables we have to re-test all the functionality to make sure there's nothing we missed. Additionally, the contracts would have to be redeployed if we also wanted to rename them to be consistent, but at current gas prices that costs over $250 so doesn't seem worth it

I'd say let's not update the 0150_auto_20200904_1505!
Instead create a new migration file
It makes it cleaner and less messier on stable as 0150 has already been executed in live

@thelostone-mc Just force-pushed with a new commit that doesn't update the migration file. Do we need a new migration though? We only updated user-facing text, not variable names/DB fields, so I don't think we need one!

@thelostone-mc
Copy link
Member

thelostone-mc commented Feb 2, 2021

@mds1 yup it should be ! cause the DB would have to updated -> which would need the migration file

@thelostone-mc thelostone-mc merged commit ed7f904 into gitcoinco:stable Feb 2, 2021
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.

4 participants