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

Clean-up and tweaks of the firehose project. #117

Merged
merged 4 commits into from
Jun 29, 2023
Merged

Clean-up and tweaks of the firehose project. #117

merged 4 commits into from
Jun 29, 2023

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Jun 27, 2023

Cleaned up and reduced RegExp use.
Simplified some functions.
Upped SDK constraint to 3.0.0, to use firstOrNull from SDK.

@@ -4,69 +4,64 @@

import 'dart:io';

import 'package:collection/collection.dart';

class Changelog {
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire class can be optimized to not read the file from scratch on every operation.
Unless it's deliberately doing it to avoid using stale data, it probably should cache the List<String> lines and/or List<_Section> sections.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that worrying about stale data was not a use-case here; some simple caching seems reasonable.

return null;
}

String? get latestHeading {
var sections = _parseSections();
// Remove all leading "#"
return sections.firstOrNull?.title.replaceAll(RegExp(r'^#*'), '').trim();
Copy link
Member Author

Choose a reason for hiding this comment

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

The only lines we accept below as title lines start with ## , so it's overkille to match any number of #s here.

.transform(const LineSplitter())
.listen((line) => stderr
..write(' ')
..writeln(line));
Copy link
Member Author

Choose a reason for hiding this comment

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

Just don't allocate extra strings unnecessarily.

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.

Thanks for the PR! We'll need to fix a test - make sure the regex is covering what we expect. And merging main into this PR should address the mono_repo related failure.

@@ -4,69 +4,64 @@

import 'dart:io';

import 'package:collection/collection.dart';

class Changelog {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that worrying about stale data was not a use-case here; some simple caching seems reasonable.

Cleaned up and reduced RegExp use.
Simplified some functions.
Upped SDK constraint to 3.0.0, to use `firstOrNull` from SDK.
@github-actions
Copy link

github-actions bot commented Jun 28, 2023

PR Health

Package publish validation ✔️

Details
Package Version Status
package:firehose 0.3.19 ready to publish
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.

@github-actions
Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:firehose 0.3.19 ready to publish firehose-v0.3.19
package:dart_flutter_team_lints 1.0.0 already published at pub.dev

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

@lrhn
Copy link
Member Author

lrhn commented Jun 28, 2023

Merged to head of main.
Which test is it that you want to add?

Always document what a RegExp matches in prose.
They are *not* readable.
@devoncarew
Copy link
Member

Merged to head of main. Which test is it that you want to add?

Sorry, for "We'll need to fix a test - make sure the regex is covering what we expect", I meant that the regex change was breaking a test; it looks like the test is no longer failing though.

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.

Feel free to publish this (via the automation link) after the PR lands.

@lrhn
Copy link
Member Author

lrhn commented Jun 29, 2023

Super, thanks.

@lrhn lrhn merged commit 19fa443 into main Jun 29, 2023
17 checks passed
@lrhn lrhn deleted the clean-up-firehose branch June 29, 2023 12:29
@devoncarew
Copy link
Member

Just tagging the commit (w/ the well-formed tag, as listed in #117 (comment)) works. However the general process is to create a github release, which also creates a tag. And, mush of that is automated - the hyperlink from the above takes you to the releases page w/ all the info for the release filled out (the name, releases notes, and tag to use).

Docs are here:https://github.com/dart-lang/ecosystem/wiki/Publishing-automation (but should be updated to mention the link that firehose creates).

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