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

Core categories expansion #25

Merged
merged 7 commits into from
Mar 9, 2018
Merged

Core categories expansion #25

merged 7 commits into from
Mar 9, 2018

Conversation

drekar
Copy link
Contributor

@drekar drekar commented Feb 12, 2018

@joshuarubin
Copy link
Contributor

Yeah, this looks good as long as this is the approach we decide on long term. We'll have to ensure that our category ids match legacy stuff that @sesq is working on. Let's wait to merge until we have a joint plan.

Copy link
Contributor

@robert-uhl robert-uhl left a comment

Choose a reason for hiding this comment

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

LGTM. I concur with @joshuarubin's comments.

@drekar
Copy link
Contributor Author

drekar commented Feb 13, 2018

After speaking with Ops, it looks like we want these categories not just to be available for use in Legacy, but to be immediately available in Legacy from the get-go. This presents a challenge because 1) adding new categories in legacy would require a nontrivial amount of development time and
2) we want to keep the codes equivalent in the two systems such that they remain being compatible
To facilitate this, the recommendation from @lnabarretezvelo is to redefine existing categories as such:

  1. Reuse financeopt_4; make it cryptocurrency
  2. Reuse netbeg_4; cryptojacking
  3. Combine the four cat_ids that belong to Javascript, C/C++, Java, Visual Basic into one cat_id called "Programming Languages".
  4. Then we have 4 spare; one can be used as blockchain. (edited)
  5. Terrorism in legacy is being put into hate_4.
    This suggestion is being confirmed with the rest of Ops and Jay, so this PR is on hold until then, as the change will need to be backported into this package.
    This presents the risk that any existing clients who have hard-coded logic dependent on these repurposed category IDs (rather than category short names) will see unexpected behavior.

Copy link
Contributor

@joshuarubin joshuarubin left a comment

Choose a reason for hiding this comment

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

category.go needs to be updated with the "long" description of the categories.
schema.graphql needs to be updated with the additions as well.

category.proto Outdated
@@ -495,4 +495,10 @@ enum Category {
KIDTRAVEL_4 = 10487;
UKTRAVEL_4 = 10488;
TRAVEL_4 = 10489;

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't yet know that these are the final names of the categories, or that their IDs match legacy.

Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage remained the same at 9.786% when pulling 3143901 on core-categories-expansion into a055c76 on develop.

@joshuarubin
Copy link
Contributor

@drekar @jbriggs-zvelo @zvbuhl can you take a look at these changes before I merge please?

Joshua Rubin added 4 commits February 28, 2018 01:50
Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
@drekar
Copy link
Contributor Author

drekar commented Mar 2, 2018

This item is still on hold pending confirmation from Ops that the new category IDs will append vs. mutate existing. I have just been informed that they will most likely be appending to the end in alignment with the code changes alredy made herein, so we shouldn't need to do any more except for add the full names as @joshuarubin mentions.

@drekar
Copy link
Contributor Author

drekar commented Mar 2, 2018

Thanks @joshuarubin for the update to add the long names and update the api metadata. When we get the final OK from Ops confirming these IDs match theirs in legacy, this PR should be ready to merge!

@drekar drekar merged commit 28363cd into develop Mar 9, 2018
@drekar drekar deleted the core-categories-expansion branch March 14, 2018 16:57
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.

5 participants