Skip to content

Conversation

@dylanwh
Copy link
Member

@dylanwh dylanwh commented Mar 18, 2018

Part of this was previously commited to unstable, but that wasn't reviewed by anyone and wasn't complete...

@CyberShadow
Copy link
Member

I'm not yet familiar with the code base to review this adequately, but I'll try to have a closer look tomorrow :)

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

I had to break up the first commit into several to look at it piecewise. Looks like this is 90% of moving code around. Would be prudent to review all usages of moved symbols to ensure they're not affected.

Another thing is that it would make sense to remove the "BMO - " markers from comments at sites using features that are being moved to core. We can then use the remaining instances as a signal of what else needs to be done in core for further separation from BMO stuff.

Copy link
Member

Choose a reason for hiding this comment

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

Guessing it's OK to just drop this migration code?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is supposed to be always_fileable_groups (one l).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants