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

Make Changelog class eagerly load and parse file. #120

Merged
merged 4 commits into from
Jun 30, 2023
Merged

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Jun 29, 2023

Rather than reading and parsing the file on each access, the class now eagerly reads the file in the constructor, and immediately parses it into sections.

Since the section parsing is incredibly simple, the overhead of splitting into sections should be minimal compared to loading the file from disk.
Also, the existing code read the entire file, even if it only needs the newest entry of the changelog, so parsing the entire file once and for all should not be significantly slower, and should be much faster on repeated access.

There should be no reason to create a Changelog and not query it at all.

Rather than reading and parsing the file on each access,
the class now eagerly reads the file in the constructor,
and immediately parses it into sections.

Since the section parsing is incredibly simple, the overhead
of splitting into sections should be minimal compared to loading
the file from disk.
Also, the existing code read the entire file, even if it only
needs the newest entry of the changelog, so parsing the entire
file once and for all should not be significantly slower,
and should be much faster on repeated access.

There should be no reason to create a Changelog and not query
it at all.
@github-actions
Copy link

github-actions bot commented Jun 29, 2023

PR Health

Package publish validation ✔️

Details
Package Version Status
package:firehose 0.3.20-wip version 0.3.20-wip is pre-release; no publish necessary
package:dart_flutter_team_lints 1.0.0 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

License Headers ✔️

Details
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Changelog Entry ✔️

Details
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -1,3 +1,6 @@
## 0.3.20
Copy link
Member

Choose a reason for hiding this comment

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

Let's stage this change for a future publish - I suspect that this is not user-facing enough to need a publish by itself.

Suggested change
## 0.3.20
## 0.3.20-wip

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, and got an error: "error: pubspec version (0.3.20-wip) and changelog (0.3.20) don't agree"

I've been thinking about that. I originally thought it was a bug.
It's probably correct, and the CHANGELOG.md should be updated to ## 0.3.20-wip.

Reasoning: When working on a package, between releases, every change may force an increment over the previous release. It's important to track that, but also to track, unambiguously, what the most recent release version was, so that I won't do two increments of the minor version just because I do two individual changes between releases. Only increment minor version if it's not already incremented, which you only know if you know what it was before being incremented the first time.

By having -wip on the CHANGELOG entry until release, we can use the most recent non -wip entry in the changelog to find the version of the most recent release.

It does mean you have to remove the -wip from both pubspec.yaml and CHANGELOG.md before releasing (but this precise validation will catch it if they differ, so it's doing good work here.)

So I guess I'd want a tool to do the updating for me:

  • repo-version -wip +patch ensures the current version is at least a patch increment over the most recent release, and adds --wip to the version in both CHANGELOG.md and pubspec.yaml.
  • repo-version +minor ensures the current version is at least a minor increment over the most recent release, and removes any pre-release tags.
  • repo-version -wip adds -wip to the current version again. In case you forgot it in the line above.

Both keep the CHANGELOG and pubspec in sync, and add an entry to the changelog if there isn't one for the current pubspec version.

Is that a tool we already have?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so the tool currently just validates that the changelog and pubspec versions exactly agree. -wip indicates that the changes in that section haven't been published. As a manual process (by review or other), we do want a new entry for every user-facing change. A new section has to be added if it's the first change after a publish. We'll drop the -wip suffix once we're ready to publish what we have (we may batch up several changes, or just publish on the first change depending on various factors).

There probably is more tooling we could do here; things like helping you create the content of the PR (adding a new changelog entry; rev'ing the pubspec version). That would be independent of general PR validation. Is that what you were thinking of?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is. When two places have to be in sync, it's always nice to have a tool to do the updating.
And it's always been a (smallish) problem that to update the patch or minor version, you need to know whether it's already been incremented since the last release or not. And sometimes someone increments again, even if they don't need to, and you accidentally skip a version

Maybe it's not a real issue, if everybody is consistent.
If you see 1.3.4-wip, with a non-zero patch, you should assume that the previous release was 1.3.3.
If you see 1.3.0-wip, you should assume the previous release was 1.2.x.

The real problem might just have been people being inconsistent.
But that's again where a tool could help.

So I propose (and, heck, might want to write myself):

  • repo-version -thetag checks if the existing version has a pre-release tag, and if so, changes it to thetag. If not, increments patch version and adds tag.
  • repo-version +patch checks if the existing version has a pre-release tag. If so, assumes at least one patch increment has already been made. If not it increments the patch level.
  • repo-version +minor checks if the existing version has a pre-release tag. If so, assumes at least one patch increment has already been made. If the patch value is zero, it assumes a minor version increment has already been made. Otherwise it increments minor version and zeros patch version, and keeps any the existing pre-release tag.
    repo-version +major works too, similarly.

It's based on version in pubspec.yaml.
It then checks if the CHANGELOG starts with an entry for the new version.
If not, and changing an existing pre-release version, it checks if CHANGELOG already has (starts with) an entry for the previous version, and updates the version in the entry.
Otherwise it adds a new, empty, entry for the new version.

That actually sounds pretty doable. The only really tricky part is updating the pubspec.yaml file in-place.
We already have classes for reading them.

I'll try to do this :)

pkgs/firehose/pubspec.yaml Outdated Show resolved Hide resolved
lrhn and others added 2 commits June 30, 2023 13:03
Co-authored-by: Devon Carew <devoncarew@google.com>
@lrhn lrhn merged commit cf02b4a into main Jun 30, 2023
@lrhn lrhn deleted the lrn-firehose-cache branch June 30, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants