[go_router_builder] Removes path_to_regexp from the dependencies
#4517
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
This file is very much inspired by path_to_regexp library, I don't know if I'm allowed to do that
There was a problem hiding this comment.
What does "inspired by" mean? If you read the source of that package as part of writing this implementation, then no, that would disqualify you from contributing an implementation (in which case this PR should be closed).
There was a problem hiding this comment.
I did read the package source code to understand what was actually happening and how go_router_builder was using it.
I guess the PR cannot be merged as it is right now. Before closing it, let me try to clean it out and re-write something else.
But go_router_builder already has its methods built around it so it might end up looking similar anyway
There was a problem hiding this comment.
Before closing it, let me try to clean it out and re-write something else.
You cannot unsee the implementation, so any implementation you write will not be a clean room implementation, and would not satisfy licensing requirements.
Someone else will need to do this.
There was a problem hiding this comment.
So I cannot work on this issue anymore? 😕
What if I find another way to code it that is not similar to the package? Would that work?
There was a problem hiding this comment.
So I cannot work on this issue anymore? 😕
That is correct. We need a clean room implementation of the functionality this package needs, and you cannot provide that.
There was a problem hiding this comment.
That is correct. We need a clean room implementation of the functionality this package needs, and you cannot provide that.
Even if as I suggested the implementation's logic is completely different? What would be the issue with that since it would explicitly not be inspired by the package?
There was a problem hiding this comment.
Even if as I suggested the implementation's logic is completely different? What would be the issue with that since it would explicitly not be inspired by the package?
I'm not a lawyer, and I have no interest in unnecessarily involving lawyers in the process of fixing the referenced issue.
|
I believe this PR can have a no-test exemption. This is a revamp and the current tests should cover it already |
|
we already have an implementation of the path util https://github.com/flutter/packages/blob/main/packages/go_router/lib/src/path_utils.dart you can just copy over |
This is what I started to do at first but go_router_builder's needs are simpler so I felt it was overkilled. I can do that and stip out |
|
If you start from our code and do nothing but remove functionality, that should be fine, since you aren't creating anything. If you would need to write anything new, you should not, and in that case someone else should do it per the discussion above. |
|
@chunhtai @stuartmorgan I've made another PR #4524 that should comply with what you want. |
Fixes flutter/flutter#130817
Relates to flutter/flutter#122713
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.