[flutter_tools] fix version tag v stripping
#55385
Conversation
fluttergithubbot
added
tool
|
@peterlauri this approach seems good, so if you're interested in working on please let us know |
|
Hi, yes. I just need to figure out my the development environment :) Want
to add a test that proves the problem as well...
…On Thu, Apr 23, 2020 at 8:43 PM Jonah Williams ***@***.***> wrote:
@peterlauri <https://github.com/peterlauri> this approach seems good, so
if you're interested in working on please let us know
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#55385 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA22EQLOELP6UTTTZVN2ECLROB45XANCNFSM4MOKQHZQ>
.
|
Sounds great. You will want to add a test to this file: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/commands.shard/hermetic/version_test.dart. Let me know if you need any additional help. I was going to try to patch this today, but if you can land this PR today it's all your's :) |
|
@christopherfujino my biggest problem is to setup VScode to follow the styling guides. Any hint on a settings file or something? :) |
|
To be honest, I don't know if I followed the style guides... I'll use Nano and fix the test styling for now, as VScode formatts plenty of things :) |
|
To confirm it was broken in the first place, revert the fix, and run |
Sorry I just saw this now. Unfortunately, you cannot use autoformatting to follow our style guide. |
|
I'm looking at the test now... |
|
@christopherfujino personally I would have refactored and parameterised the tests... But I gotta learn more about dart tests, coming from python/pytest background where I know exactly how to do it. |
|
@peterlauri this looks great! Thanks so much for this contribution! |
christopherfujino
changed the title
v stripping
This reverts commit c91a3a7.
This reverts commit c91a3a7.
|
Shucks, unfortunately this failed testing on post-submit and was reverted. If I have free cycles today I'll investigate... |
|
@christopherfujino anything I can do to help? |
|
Sure, you could investigate why this test passed on pre-submit but failed on post: https://cirrus-ci.com/task/6113630626250752?command=main#L336 |
|
@christopherfujino sure, I'm on it. Can confirm after merging master prior to the merge that the test fails... |
|
@christopherfujino Are tags like 1.18.0-dev.5.0 expected to be allowed with 'flutter version'? Or only tags like 1.18.0-6.0.pre? |
The short answer is no, the tags of the form |
|
Ahh, I see, this must have passed on pre-submit because it was branched before the change was made. |
|
@christopherfujino I know that is changed, but as the old tags exist should flutter version allow them? It is a fairly easy fix to allow both, and I'm happy to do that fix. Your call... |
|
Another instance that would have been caught by #53366 |
|
Sorry, you're right that |
|
Yes, I'll add test for both dev and pre and fix the implementation. Easy
one :)
…On Fri, Apr 24, 2020 at 11:59 PM Christopher Fujino < ***@***.***> wrote:
Sorry, you're right that flutter version 1.18.0-dev.5.0 should be
allowed. But I think the test passed pre-submit because it wasn't rebased
off of upstream.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#55385 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA22EQLU57LLJZTEWQXBVTLROH4R3ANCNFSM4MOKQHZQ>
.
|
Description
This fixes an incorrect string replacement that is intended to strip the first v, for example:
v1.0.0 => 1.0.0
1.0.0.dev.0 => 1.0.0.dev.0 (actual 1.0.0.de.0)
Related Issues
Fixes #55421 and #55383
Tests
Added test "dev version switch prompt is accepted" that checks that dev versions can be requested in the flutter version command.
Also fixed the test it self, as it had the same defect as the implementation it self.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.