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

Use floor_annotation git ref during development #298

Closed
wants to merge 4 commits into from

Conversation

vitusortner
Copy link
Collaborator

@vitusortner vitusortner commented Mar 28, 2020

This allows using snapshot versions of the library. We can't use local paths for snapshot versions as Pub disallows relative paths. I've decided for this solution as we won't change floor_annotation that frequently.

Closes #288

@vitusortner vitusortner requested a review from mqus March 28, 2020 11:03
@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #298 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #298   +/-   ##
========================================
  Coverage    81.58%   81.58%           
========================================
  Files           57       57           
  Lines         1477     1477           
========================================
  Hits          1205     1205           
  Misses         272      272           
Flag Coverage Δ
#floor 81.58% <ø> (ø)
#floor_generator 81.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce10935...52efba6. Read the comment docs.

Copy link
Collaborator

@mqus mqus left a comment

Choose a reason for hiding this comment

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

LGTM

@vitusortner
Copy link
Collaborator Author

I just realized that with these changes, we won't be able to use newly added annotations until they're available on develop. This would mean that we'll aways be required to push the annotations to develop before, for instance, our CI can use them.

Do you have another idea how to solve the "../floor_annotation/" is a relative path, but this isn't a local pubspec. problem? @mqus

@mqus
Copy link
Collaborator

mqus commented Mar 28, 2020

Maybe...
I see at least three options, all have their downsides:

  1. We could split PRs which change annotations in two subsequent PRs, in which one introduces the change to floor_annotation (and can't really be tested) and the following one updates the generator/floor library... this could happen concurrently by referencing the annotation-PR branch while developing in the other branch and switching that after the annotation PR was merged....
    If i understand it right then this is what you already proposed.

  2. Any PR changing the annotations has to change all references in the pubspec to its PR branch, which will persist until the release or another commit updates the reference again.

  3. We keep the current relative annotations and release often. Everyone who still wants to use the develop branch has to use a local copy by either using git submodules or just extracting a snapshot into their sources.

The downsides of 3 were already discussed, 1 and 2 are pretty complex and we would have to apply them to all drive-by PRs. Otoh, changes to floor_annotation seem to be fairly rare, so this should only be necessary for a handful of PRs.

Personally, I'd like to avoid complexities in the PR process so I'm advocating for 3. But I don't have a strong opinion on this, so this would be up to you ;-)

@vitusortner
Copy link
Collaborator Author

I'll close this PR for now and remove the snapshot section from the README.

@vitusortner vitusortner closed this Apr 3, 2020
@vitusortner vitusortner deleted the snapshot-versions branch April 3, 2020 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Snapshot dependencies don't work
2 participants