fix saving of comment ids to disk

[jakemac53]

See the failure here https://github.com/dart-lang/build/actions/runs/7463208455/job/20307339352?pr=3638#step:9:3.

Changes findCommentId to again return an int? instead of an IssueComment?.

Alternatively, we could rename this to findComment, and return the entire comment, but we only ever use the ID.

We just call toString() on the return value of this function and serialize it to disk, so when it changed to an IssueComment it no longer serialized the ID.

[github-actions]

Package publishing

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

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

[github-actions]

PR Health

Breaking changes ⚠️

Details

Package	Change	Current Version	New Version	Needed Version	Looking good?
firehose	Breaking	0.5.0	0.5.1	0.6.0 Got "0.5.1" expected >= "0.6.0" (breaking changes)	⚠️

This check can be disabled by tagging the PR with skip-breaking-check

Changelog Entry ✔️

Details

Package	Changed Files

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

Coverage ✔️

Details

File	Coverage
pkgs/firehose/lib/src/github.dart	💚 62 % ⬆️ 1 %

This check for test coverage is informational (issues shown here will not fail the PR).

License Headers ✔️

Details

// Copyright (c) 2024, 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.

Package publish validation ✔️

Details

Package	Version	Status
package:firehose	0.5.1	ready to publish
package:dart_flutter_team_lints	2.1.1	already published at pub.dev

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

[jakemac53]

I am not sure why this is being flagged as breaking, given this is a private API only?

[parlough]

I am not sure why this is being flagged as breaking, given this is a private API only?

\cc @mosuem

[mosuem]

I am not sure why this is being flagged as breaking, given this is a private API only?

The log says

{
  "label": "Class GithubApi",
  "children": [
    {
      "label": "Method findCommentId",
      "children": [
        {
          "changeDescription": "Return type changed. Future<IssueComment?> -> Future<int?>",
          "changeCode": "CE09",
          "isBreaking": true,
          "type": "minor"
        }
      ]
    }
  ]
}

Probably because files in lib/src are technically part of the public API, even if importing them is discouraged.
cc @devmil

[devmil]

I am not sure why this is being flagged as breaking, given this is a private API only?

[...]

Probably because files in lib/src are technically part of the public API, even if importing them is discouraged.

cc @devmil

Will have a look.

Dart files in src are considered private as long as they are not exported outside, so that should not be the reason.
Is the modified type used in some public API? Then it would be considered implicitly public as client code could interact with it without importing it

[jakemac53]

Dart files in src are considered private as long as they are not exported outside, so that should not be the reason.
Is the modified type used in some public API? Then it would be considered implicitly public as client code could interact with it without importing it

I don't see it being exposed (there are no exports from lib/firehose.dart). However, I see that Firehose.verify takes a GithubApi typed parameter, even though there is no way to see that type, which seems like an oversight.

[devmil]

Dart files in src are considered private as long as they are not exported outside, so that should not be the reason.
Is the modified type used in some public API? Then it would be considered implicitly public as client code could interact with it without importing it

I don't see it being exposed (there are no exports from lib/firehose.dart). However, I see that Firehose.verify takes a GithubApi typed parameter, even though there is no way to see that type, which seems like an oversight.

I think this method is why dart_apitool considers it part of the public API:

ecosystem/pkgs/firehose/lib/firehose.dart

Line 93 in 244a28d

Future<VerificationResults> verify(GithubApi github) async {

Technically it can not break consumer code (and the method can't be called with something meaningful) so we could extend dart_apitool to consider the data flow direction for this as well.

For now you probably can bypass it by making the verify method private. No, you can't as this is another breaking change :/

[jakemac53]

Technically it can not break consumer code (and the method can't be called with something meaningful) so we could extend dart_apitool to consider the data flow direction for this as well.

I think that is reasonable, but I do also think it indicates a mistake on the part of the API.

There should not be public apis, which take parameters whose types are from private libraries in the same package (or another package, for that matter). All types exposed through public APIs, should also be public (although they don't need to be exported from the same exact library).

[jakemac53]

Fwiw, I would be happy to make the breaking change as well, to make verify private, whatever you all think is appropriate.

[jakemac53]

Going to go ahead and land as is given the two approvals

[devmil]

I think that is reasonable, but I do also think it indicates a mistake on the part of the API.

There should not be public apis, which take parameters whose types are from private libraries in the same package (or another package, for that matter). All types exposed through public APIs, should also be public (although they don't need to be exported from the same exact library).

@mosuem fyi. dart_apitool already has a flag to check for missing exports in the "extract" command: --set-exit-on-missing-export. We could think about another option that is not as harsh and include this information in the generated report.

So that's enough hijacking of a closed PR :)