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 poetry.rb functions #51956

Merged
merged 3 commits into from
May 18, 2023
Merged

Rename poetry.rb functions #51956

merged 3 commits into from
May 18, 2023

Conversation

fisher-alice
Copy link
Contributor

@fisher-alice fisher-alice commented May 17, 2023

This PR renames two functions within poetry.rb:

  • from poems_for_subtype to poem_list_for_standalone_app
  • from poem_keys_for subtype to poem_list_keys_for_standalone_app.

Poetry has 3 subtypes: 'poetry', 'poetry_hoc', and 'time_capsule'. However, in poetry.rb, subtype is referred to as standalone_app.
The two functions mentioned above are renamed to clarify their purpose and maintain more consistency in naming.
This is a follow-up to this PR.

Links

Testing story

I tested locally by opening new 'poetry', 'time_capsule', and 'poetry_hoc' projects and confirming there were no changes to the poem lists. I also viewed poem lists in the Poetry HOC levels.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@fisher-alice fisher-alice marked this pull request as ready for review May 18, 2023 15:07
@fisher-alice fisher-alice requested review from molly-moen, mikeharv and a team May 18, 2023 15:07
Copy link
Contributor

@mikeharv mikeharv left a comment

Choose a reason for hiding this comment

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

The changes here look good, especially after chatting through with Alice offline.
The term standalone_app initially threw me off because I was expecting that to refer to "bigger" apps like spritelab vs applab. Seeing that we only use this term in the context of SpriteLab/Poetry has feeling good that we're not overloading the term to mean different things in different context.

@molly-moen, I'm thinking you chose this label when we first looked into subtypes, so I know I've seen it a bunch already. Could you remind me how you interpret the term "standalone app"?

@molly-moen
Copy link
Contributor

molly-moen commented May 18, 2023

@molly-moen, I'm thinking you chose this label when we first looked into subtypes, so I know I've seen it a bunch already. Could you remind me how you interpret the term "standalone app"?

@mikeharv I interpret "standalone app" as the configuration for a project that can be run via a /projects/<standalone-app-name>/<channel-id> url. It is actually a term used on the frontend as well, and is defined as such. With some refactoring for the new projects client I ended up adding a new term for this, which is project_type. I am now wondering if I should change that to standalone_app_name to be consistent everywhere. project_type is a bit of a clearer term IMO, but it is new so going with consistency is probably better.

@mikeharv
Copy link
Contributor

@molly-moen Thanks for explaining!
So it seems like the "standalone app" could be any thing from AppLab to Time Capsule and there's no real hierarchy of labs/subtypes present. I was thinking that the standalone app for things like Time Capsule and Story would be Poetry and Sprite Lab respectively. It's helpful to think about the app name mapping evenly to the project types we support.

project_type is a bit of a clearer term IMO, but it is new so going with consistency is probably better.

Agreed on both points!

# not in the list of poems for the subtype.
def sanitize_default_poem
self.default_poem = nil if Poetry.subtypes_with_poems.exclude?(standalone_app_name) ||
Poetry.poem_keys_for_subtype(standalone_app_name).exclude?(default_poem)
Poetry.poem_list_keys_for_standalone_app(standalone_app_name).exclude?(default_poem)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it would be a bit cleaner to name these poem_keys_for_standalone_app and poems_for_standalone_app, because a plural implies a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - sounds good. Will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And just to clarify, did you mean poems_keys_for_standalone_app or poem_keys_for_standalone_app?

Copy link
Contributor

Choose a reason for hiding this comment

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

poem_keys, because the list is of keys

Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

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

LGTM, one naming nit but it's a matter of preference :)

@fisher-alice fisher-alice merged commit 8447ea3 into staging May 18, 2023
2 checks passed
@fisher-alice fisher-alice deleted the alice/poetry_rename branch May 18, 2023 20:01
pablo-code-org pushed a commit that referenced this pull request May 25, 2023
Rename poetry.rb functions
snickell pushed a commit that referenced this pull request Feb 3, 2024
Rename poetry.rb functions
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.

None yet

3 participants