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

Generate deprecations list from the language repo #2253

Merged
merged 9 commits into from
May 29, 2024
Merged

Conversation

jathak
Copy link
Member

@jathak jathak commented May 21, 2024

See sass/sass#3872.

This updates the Deprecation enum to be generated from spec/deprecations.yaml in the language repo.

This updates the Deprecation enum to be generated from
spec/deprecations.yaml in the language repo.
@jathak jathak requested a review from nex3 May 21, 2024 23:08
CHANGELOG.md Outdated
Comment on lines 6 to 7
`Deprecation.duplicateVarFlags` to make it consistent with the
`duplicate-var-flags` name used on the command line and in the JS API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing indentation

.gitignore Outdated
*.pb*.dart
*.g.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little odd, since the async files are also generated but don't have a marked filename. Might it be simpler to just explicitly ignore the deprecation.dart file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking in terms of having something to distinguish generated code that's not checked in, and I'd seen the .g.dart pattern with some other Dart projects, but maybe since deprecation.dart is the only file like that here (other than the protobuf files, which use their own extension) just ignoring it explicitly is better.

import 'package:dart_style/dart_style.dart';
import 'package:yaml/yaml.dart';

void updateDeprecationFile(File yamlFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more consistent with other libraries to define the task here directly.


void updateDeprecationFile(File yamlFile) {
var yamlText = yamlFile.readAsStringSync();
var data = loadYaml(yamlText) as Map;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass in the source URL here as well.

Checking this in makes it possible to checkout and use the source
without needing grinder, and there's not really any case where we would
need to add or update a dependency in the language repo without having
some sort of corresponding PR in this repo.
@jathak jathak requested a review from nex3 May 23, 2024 18:45
@jathak
Copy link
Member Author

jathak commented May 23, 2024

Based on offline discussions, I decided to change this to check in the generated deprecations file since there shouldn't really be a case where we need to update the deprecations list in the language repo without making some sort of corresponding change to the compiler, and checking it in makes it easier to use the compiler code internally without also needing to pull in the language repo and grinder.

fail('${deprecations.dartPath} is out-of-date.\n'
'Run `dart run grinder` to update it.');
}
}, testOn: "!windows");
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth including the above comment here as well, or moving this condition onto a group that contains both up-to-date tests.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having a template and a source file, could it make sense to just allow the source file to be edited directly and only have a specific subsection of it get (re)generated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. Now that the deprecations file is checked in, there's no reason not to just generate a section of it. Fixed.

@@ -21,7 +20,7 @@ Future<void> doubleCheckBeforeRelease() async {
fail("GITHUB_REF $ref is different than pubspec version ${pkg.version}.");
}

if (listEquals(pkg.version.preRelease, ["dev"])) {
if (const ListEquality<Object?>().equals(pkg.version.preRelease, ["dev"])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't listEquals() work here anymore?

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's defined within lib/src/utils.dart, which depends on the Sass AST, so we can't import it here

@jathak jathak merged commit 1073c7b into main May 29, 2024
34 checks passed
@jathak jathak deleted the generated-deprecations branch May 29, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants