-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add support for latest release #269
Add support for latest release #269
Conversation
Haven't read the code yet, but I had an unanswered question in the other pull request. Let's say |
This states "Execute |
Yeah, so if we want to provide a more true latest release then we need to do the reconciliation which would require a bigger refactoring, I tried walking down that path in the other PR but decided to bail out. If there is a big interest in this we can certainly do it and we can leverage semver to do the heavy lifting by sorting the versions. Regarding OTP and it’s branch versioning scheme, that can be treated as SemVer build meta and be sorted accordingly. In the contribution guide it’s mentioned to keep changes to a minimum, so as a new contributor then I’m hesitant to rip apart most of this as it’s currently stand. |
3f89461
to
5d80cab
Compare
I don't know how that didn't get pushed the first round, but I've rebased and regenerated the dist. Hopefully, it will pass this time around. |
I understand, and thank you for being patient and walking us through your thought process. The changes mostly seem sane to me (it's touching parts of the code that are somewhat sensitive), but I think I need other reviewers to pitch in. In any case, tests are failing and it's probably due to |
Thanks, I've future-proofed the tests. |
c50ab82
to
809f03c
Compare
Failing tests don't seem to be your responsibility; it's just that |
@Schultzer, it'd be important to mention this in the README, with all the discussed constraints. Basically:
|
809f03c
to
02f2471
Compare
When you squash your changes midway through a review it makes it harder on the reviewer to follow up 😕, since all the changes will now have to be reviewed again... No biggie, just letting you know how I feel it. |
b074fa1
to
c26af22
Compare
Done, no known constraints, should be all cake from here. 🍰 |
I apologize, the whitespace bit me, I couldn’t find the commit that had it and I fat fingered git. Which ended up with me squeezing the changes. I’m a complete noob with regards to git. |
@Schultzer, I don't think we have any more planned changes. I'm thus rebasing this onto the main branch to check if the error issues go away. If they do we can expect a squash+merge soon. |
c26af22
to
4ed3595
Compare
This is Ok ✅. I'm squash+merging. You can expect this change in the next release (we want to wait until #278 is done, so we can bump minor). Feel free to come back to this and ask for the release if it's taking more than, say, a week. |
Description
This PR closes #266 and supersedes #267; where I got a bit lost in the codebase.
Closes #266