Closed SaurabhMSFT closed 10 months ago
SammyO
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
shahzaibj
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
SammyO
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
The PR description suggests something else to me. And looking at (some) files that previously lived in common4j, they're now in common.nativeauth. E.g. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2257/files#diff-accba3aa90af0abc54afa35f6316d7840452944d79747809bb7d8573c5173c23
fadidurah
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
The PR description suggests something else to me. And looking at (some) files that previously lived in common4j, they're now in common.nativeauth. E.g. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2257/files#diff-accba3aa90af0abc54afa35f6316d7840452944d79747809bb7d8573c5173c23
I think for this example at least, it's still in common4j. From a quick glance, i think this PR is correct, but perhaps it would be better to include .java in the nativeauth common4j package path, to be consistent with what already exists in common4j
SaurabhMSFT
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
The PR description suggests something else to me. And looking at (some) files that previously lived in common4j, they're now in common.nativeauth. E.g. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2257/files#diff-accba3aa90af0abc54afa35f6316d7840452944d79747809bb7d8573c5173c23
I think for this example at least, it's still in common4j. From a quick glance, i think this PR is correct, but perhaps it would be better to include .java in the nativeauth common4j package path, to be consistent with what already exists in common4j
I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
The PR description suggests something else to me. And looking at (some) files that previously lived in common4j, they're now in common.nativeauth. E.g. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2257/files#diff-accba3aa90af0abc54afa35f6316d7840452944d79747809bb7d8573c5173c23
I have not moved any file across projects. While moving the files (in the same project) which were in the package a.b.nativeauth have been moved to package a.nativeauth.b. For the files that were not in nativeauth specific package, I have tried to preserve their package name as much as possible.
SaurabhMSFT
commented
10 months ago I'd be more in favour of having dedicated native auth packages in common and common4j, rather than having everything in common, so that we're in line with existing architecture and don't diverge too far from it. @shahzaibj @fadidurah wdyt?
I think that's what this PR is already doing. I scanned it and it seems to have dedicated native auth packages in both common and common4j. So it looks good to me. Am I missing anything here?
The PR description suggests something else to me. And looking at (some) files that previously lived in common4j, they're now in common.nativeauth. E.g. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2257/files#diff-accba3aa90af0abc54afa35f6316d7840452944d79747809bb7d8573c5173c23
I think for this example at least, it's still in common4j. From a quick glance, i think this PR is correct, but perhaps it would be better to include .java in the nativeauth common4j package path, to be consistent with what already exists in common4j
I will add the java part to the package name
Native auth files in common4j and common project have been moved into com.microsoft.identity.common.nativeauth package