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

Migrate to null-safety #21

Conversation

nyarian
Copy link
Contributor

@nyarian nyarian commented Feb 3, 2021

No description provided.

@nyarian nyarian force-pushed the infrastructure/null-safety-migration branch from 8c745aa to fe52612 Compare February 3, 2021 21:54
Copy link
Owner

@Francessco121 Francessco121 left a comment

Choose a reason for hiding this comment

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

This is pretty good so far. I do have some suggestions and concerns that I added. Other than those, MSAL's source still needs to be looked at to see what properties can be safely non-nullable.

I apologize if this review seems a bit overkill or nitpicky, I'm just trying to keep the code consistent and safe. It's also my first time ever doing a code review on GitHub :). I'll probably tweak a few things myself later anyway, so don't worry too much about the nitpicks but I'd appreciate if you could change the ones I noted.

lib/src/cache_location.dart Outdated Show resolved Hide resolved
lib/src/auth_request.dart Outdated Show resolved Hide resolved
lib/src/auth_response.dart Outdated Show resolved Hide resolved
lib/src/exceptions.dart Outdated Show resolved Hide resolved
lib/src/interop/authentication_parameters.dart Outdated Show resolved Hide resolved
lib/src/user_agent_application.dart Outdated Show resolved Hide resolved
lib/src/user_agent_application.dart Outdated Show resolved Hide resolved
lib/src/user_agent_application.dart Outdated Show resolved Hide resolved
test/configuration_test.dart Outdated Show resolved Hide resolved
test/js_proxies/js_object_map_proxy_test.dart Outdated Show resolved Hide resolved
@nyarian nyarian force-pushed the infrastructure/null-safety-migration branch from d7b67c2 to 57222f3 Compare February 6, 2021 22:30
@nyarian
Copy link
Contributor Author

nyarian commented Feb 6, 2021

I believe I've fixed the issues you outlined and resolved the appropriate discussions, but there are a few discussions that are not as straightforward so I'd ask you to resolve them if you'll see the fixes done as applicable.

once you are ready to merge this, please, ping me out so I'll squash the commits before that so the git won't contain trash commits like 'apply requested PR changes'

@nyarian nyarian force-pushed the infrastructure/null-safety-migration branch from 57222f3 to fc97e94 Compare February 6, 2021 22:41
Copy link
Owner

@Francessco121 Francessco121 left a comment

Choose a reason for hiding this comment

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

Thanks, yeah this is pretty good, I just have one issue with the changes to cache_location.

I would also still like to go through the source of msal.js to determine the nullability of various properties. If you're not comfortable with that, I can take over and finish this up.

How important is it to you that this package is null-safe? Is it blocking one of your projects? I wasn't planning on releasing a null-safe version for a while until your PR came along, so if it's an issue I can get this published soon.

lib/src/cache_location.dart Outdated Show resolved Hide resolved
lib/src/configuration.dart Show resolved Hide resolved
@nyarian nyarian force-pushed the infrastructure/null-safety-migration branch from fc97e94 to 31a1185 Compare February 7, 2021 23:11
@nyarian
Copy link
Contributor Author

nyarian commented Feb 7, 2021

as much as I would like to sync msal-js properties with the ones in this library, I'm simply not qualified with either JS or TS, so I'd rather leave it for someone who is

the package is quite important for me to be null-safe, but I'm feeding from my own fork already instead of using the pub.dev version, so I have no issues with that. this contribution is just for karma points

@nyarian nyarian force-pushed the infrastructure/null-safety-migration branch from 31a1185 to 11bf099 Compare February 7, 2021 23:19
@Francessco121
Copy link
Owner

Ok, no problem. I'll finish this up soon. Thanks a ton for getting it this far!

@Francessco121 Francessco121 changed the base branch from master to nullsafety February 12, 2021 21:24
@Francessco121 Francessco121 merged commit 0fc672a into Francessco121:nullsafety Feb 12, 2021
@Francessco121 Francessco121 mentioned this pull request Feb 12, 2021
7 tasks
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.

2 participants