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

Refs #26008 - parameter type to parameter commands #418

Merged
merged 1 commit into from Apr 11, 2019

Conversation

ezr-ondrej
Copy link
Member

Adds parameter_type.

I have realized it is going to be addressed already in the middle of the process.
I have found @kgaikwad work, but as in the process, I realized we need to do a bit more anyway.

Sorry @kgaikwad for being sloppy and not looking before starting to code :(
But I would suggest merging #409 first and I will rebase my work on that one, as there is default value addressed, that I forgot and should be in other resources as well.

@ezr-ondrej ezr-ondrej changed the title Fixes #26008 - parameter type to parameter commands [WIP] Fixes #26008 - parameter type to parameter commands Apr 9, 2019
@ofedoren
Copy link
Member

ofedoren commented Apr 9, 2019

@ezr-ondrej, #409 is merged, so this needs a rebase.

@kgaikwad
Copy link
Member

kgaikwad commented Apr 9, 2019

@ezr-ondrej,
no worries! :-)
👍 for extraction of data types into a constant.

@ezr-ondrej
Copy link
Member Author

Ok, I have rebased it, basically only addition is the extraction to the KEY_TYPES.
I have thrown renaming of the parameter to type as I felt that parameter-type is unecessarily long, but is not up to me to decide and @kgaikwad solved it with long one already.

Another thought - probably for followup would be CommonParameter < AbstractParameter inheritance as we got double work, but it is not as simple, and is just twice there, so no big deal :)

@ezr-ondrej ezr-ondrej changed the title [WIP] Fixes #26008 - parameter type to parameter commands Refs #26008 - parameter type to parameter commands Apr 9, 2019
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

@ezr-ondrej, thank you for PR. If you think it's ready, I'll merge it right after your answer :)

@ezr-ondrej
Copy link
Member Author

@ofedoren yeah, I believe it's ready :)

@ofedoren
Copy link
Member

@ezr-ondrej, merging then, thanks! :)

@ofedoren ofedoren merged commit ef39dfe into theforeman:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants