Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

@MauricioCarneiro
Copy link
Contributor

fixes #171

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the declaration of this enum should be moved to variant.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we do that, we will be exposing it to the developers. Right now it's a private gamgee implementation thing. I like it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but if I follow your motivation, I think we can move this utility file into the main directory and call it VariantFieldType instead of "utils".

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a utility, and shouldn't be buried in a utilities file. With other similar enums (eg., Base, CigarOperator, etc.) we've put them in the main header. Plus there are public functions all over the place that take VariantFieldType as an argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename the file from field utils to VariantFieldType I'd be ok with keeping it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename but keep it in utils?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I meant rename and move out of utils. It's not a utility

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 78835ec on mc_rename_format_field_type_171 into 7b46c6a on master.

@MauricioCarneiro MauricioCarneiro force-pushed the mc_rename_format_field_type_171 branch from 78835ec to fac3abd Compare August 21, 2014 21:19
@MauricioCarneiro
Copy link
Contributor Author

Renamed

@MauricioCarneiro MauricioCarneiro force-pushed the mc_rename_format_field_type_171 branch from fac3abd to 66d5127 Compare August 21, 2014 21:22
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 66d5127 on mc_rename_format_field_type_171 into 7b46c6a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.24%) when pulling 66d5127 on mc_rename_format_field_type_171 into 7b46c6a on master.

MauricioCarneiro pushed a commit that referenced this pull request Aug 22, 2014
…pe_171

FormatFieldType: renamed to VariantFieldType
@MauricioCarneiro MauricioCarneiro merged commit 30fdc1f into master Aug 22, 2014
@MauricioCarneiro MauricioCarneiro deleted the mc_rename_format_field_type_171 branch August 22, 2014 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename FormatFieldType to reflect it's usage on both Shared and Individual fields

4 participants