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 applies_to_exp to visibility_expression #28

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Jul 3, 2017

applies_to_exp is unused and we can use it for storing
MiqExpression which can say whether the button will be
displayed or not.
I think that it is better-named regard to feature but I am open for other ideas how to name it.

Links

https://www.pivotaltracker.com/n/projects/1608513/stories/147780727
cc @gtanzillo

@miq-bot assign @Fryguy

@miq-bot add_label gapridashvili

@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2017

@lpichler Cannot apply the following label because they are not recognized: gapridashvili

@@ -0,0 +1,5 @@
class RenameAppliesToExpToMiqExpressionVisible < ActiveRecord::Migration[5.0]
def change
rename_column :custom_buttons, :applies_to_exp, :miq_expression_visible
Copy link
Member

Choose a reason for hiding this comment

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

visible in an adjective, so this reads funny to me. I'm thinking visibility_expression or visibility_miq_expression? Thoughts? Similar discussion in #29

Copy link
Member

Choose a reason for hiding this comment

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

@imtayadeway Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

As in the other PR, I think visibility_<thing>, where thing is one of the common words we use to designate an expression is best 👍

@lpichler lpichler force-pushed the rename_applies_to_exp_to_miq_expression_visibily branch from 5800cfa to f25a9b3 Compare July 13, 2017 08:33
@lpichler lpichler changed the title Rename applies_to_exp to miq_expression_visible Rename applies_to_exp to visbility_expression Jul 13, 2017
@lpichler lpichler changed the title Rename applies_to_exp to visbility_expression Rename applies_to_exp to visibility_expression Jul 13, 2017
@lpichler lpichler force-pushed the rename_applies_to_exp_to_miq_expression_visibily branch 2 times, most recently from ac635cd to 5bcc730 Compare July 13, 2017 08:51
applies_to_exp is unused and we can use it for storing
MiqExpression which can say whether the button will be
displayed or not.
@lpichler lpichler force-pushed the rename_applies_to_exp_to_miq_expression_visibily branch from 5bcc730 to 679046a Compare July 13, 2017 08:52
@miq-bot
Copy link
Member

miq-bot commented Jul 13, 2017

Checked commit lpichler@679046a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@lpichler
Copy link
Contributor Author

I used visibility_expression thanks @Fryguy and @imtayadeway :)

@Fryguy
Copy link
Member

Fryguy commented Jul 17, 2017

@lpichler Is there a corresponding PR on manageiq that uses this column that needs to be merged at the same time?

I need to learn to read: ManageIQ/manageiq#15501

@Fryguy Fryguy merged commit 4e8961e into ManageIQ:master Jul 17, 2017
@Fryguy Fryguy added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 17, 2017
@lpichler lpichler deleted the rename_applies_to_exp_to_miq_expression_visibily branch July 17, 2017 17:51
lpichler added a commit to lpichler/manageiq that referenced this pull request Jul 24, 2017
After renaming, rename also related column for serializing.
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.

5 participants