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

prune family names from list in script constants #49803

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Jan 13, 2023

Follow-up from https://github.com/code-dot-org/code-dot-org/pull/49755/files/d73237b9c14c3b4ea34a4ca57c56fc3999f6462a#r1069930260 . This list was misleading because it had some cruft in it, and the comment implied that new family names needed to be added to it. This PR removes the cruft and fixes the comment.

Testing story

@davidsbailey davidsbailey deleted the prune-script-constant-family-names branch February 8, 2023 07:28
@davidsbailey davidsbailey restored the prune-script-constant-family-names branch February 8, 2023 17:37
@davidsbailey davidsbailey reopened this Feb 8, 2023
@davidsbailey davidsbailey requested review from a team February 8, 2023 21:57
assert_redirected_to "/s/cats2/lessons/1/levels/1"

# next redirects to latest version in a script family
get :next, params: {script_id: 'ui-test-versioned-script'}
get :next, params: {script_id: 'cats'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Great cleanup –– this is much clearer to me, who still gets confused about family names!

@@ -17,6 +17,16 @@ module ScriptConstants
CSP17_UNIT2_NAME = 'csp2-2017'.freeze
CSP17_UNIT3_NAME = 'csp3-2017'.freeze

# CSF family names
COURSEA = 'coursea'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we reference these constants in two places, and Ruby constants always feel a bit magical to me :D.

To check my understanding: we import ScriptConstants in our Unit model, so other places can access the COURSEA constant via Unit::COURSEA. And that syntax stays consistent whether or not this constant is inside of an array (like FAMILY_NAMES) or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct! To be slightly more specific we include ScriptConstants into the Unit class

Copy link
Member Author

Choose a reason for hiding this comment

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

Including a module into a class makes all of the module's members accessible on the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, include is different from import? So the "module's members" include all constants, regardless of how they're stored (or what kind of data structure they're stored in).

Anyways, I think I'm getting it –– thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, for modules require and include are different. this page describes what happens when you include a module: https://ruby-doc.org/core-2.6.3/Module.html

you can also require a module, in which case it behaves a lot like requiring a class, e.g.:

require 'cdo/metrics_helper'

there are many blog posts on the internet about this, the main thing I would suggest is that you search for "ruby" and not "rails" because this is actually plan ruby stuff not specific to rails.

Copy link
Contributor

@megcrenshaw megcrenshaw left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@davidsbailey davidsbailey merged commit 78a3b72 into staging Feb 9, 2023
@davidsbailey davidsbailey deleted the prune-script-constant-family-names branch February 9, 2023 16:40
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