-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Pubcommon id and shared id submodules: merge into one submodule #6808
Conversation
This pull request fixes 1 alert when merging 2ee857b into 22ad110 - view on LGTM.com fixed alerts:
|
@NothingIsMatter prebid 5 was updated just now, can you repull the branch / merge conflicts? |
@patmmccann, @NothingIsMatter is in the EU, is this something that needs to be addressed today, or can it wait until Monday? |
It can definitely wait, the 5.0 release is two weeks out |
This pull request fixes 1 alert when merging 9e1bfee into 6957925 - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things to take a look at here. Also it seems as though perhaps some of the files were overlooked in this update. The previous change consisted of 31 files whereas this update has only modified 25. Just doing a quick comparison, I can see that openx and ix adapters no longer have the update to remove the sharedId references. I would suggest going over this again to make sure all necessary updates are included in this version as well
* @param {SubmoduleParams} [config] | ||
* @param {ConsentData|undefined} consentData | ||
* @param {Object} storedId existing id | ||
* @returns {IdResponse|undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the extendId function comments are included but the pubCommonId function itself was not placed in here. Was this intentional? The domainOverride function was also not included as it was in the last PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that wasn`t intentional, will remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -1,77 +0,0 @@ | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this module test being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to #6640:
remove the following sharedid files
modules/sharedIdSystem.js
test/spec/modules/sharedIdSystem_spec.js
modules/sharedIdSystem.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@mmoschovas added comments, updated the branch |
This pull request fixes 1 alert when merging e30ad8a into 6957925 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 62d6193 into 6957925 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging ec41a92 into 6957925 - view on LGTM.com fixed alerts:
|
@NothingIsMatter I think there has been some confusion here most likely due to the nature of the ticket but quite possibly because of my comments. I just want to make sure there is a shared understanding here:
|
|
This pull request fixes 1 alert when merging 6d4096d into 6957925 - view on LGTM.com fixed alerts:
|
@mmoschovas mind re reviewing we've made the requested changes. |
@jdwieland8282 are the unit tests going to be updated as well? It still looks like the spec file is testing the deprecated pubCommonId file |
@@ -25,7 +25,6 @@ import { | |||
import {server} from 'test/mocks/xhr.js'; | |||
import find from 'core-js-pure/features/array/find.js'; | |||
import {unifiedIdSubmodule} from 'modules/unifiedIdSystem.js'; | |||
import {pubCommonIdSubmodule} from 'modules/pubCommonIdSystem.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmoschovas pubCommonIdSystem.js is removed from the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SKOCHERI I'm referring to test/spec/modules/sharedIdSystem_spec.js which is using the pubCommonId.js functions not sharedIdSystem.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmoschovas The test/spec/modules/sharedIdSystem_spec.js is updated. Can you review it?
This pull request fixes 1 alert when merging 8ca32a1 into fe175cd - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@NothingIsMatter merge conflict showed up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request fixes 1 alert when merging 0c7e5f6 into 61c494a - view on LGTM.com fixed alerts:
|
Type of change
Description of change
Merge pubcid and sharedid
Other information
Issue - #6640