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

Fix generated_plugin_registrant.dart not being ignored #52

Merged
merged 4 commits into from
May 30, 2021
Merged

Fix generated_plugin_registrant.dart not being ignored #52

merged 4 commits into from
May 30, 2021

Conversation

bartekpacia
Copy link
Contributor

@bartekpacia bartekpacia commented May 10, 2021

Wow, finally! I've lost a few hours in the last few days tracking a very strange error with this package, which led to my GitHub Action (which was using import_sorter to check if imports are sorted) failing for seemingly no reason.

What was wrong

generated_plugin_registrant.dart file was not being ignored by import_sorter - the logic was flawed. We want to check if dependencies in pubspec.yaml contain flutter, not flutter/!

Also, if import_sorter failed, it now prints which file caused that.

Now it's fixed and works just fine. Please release v4.5.2 ASAP, I want to be able to use it finally in my GH Actions :D

Trivia

Why were my GitHub Actions failing?
Because I've set up import_sorter to run automatically on save on my computers. It formatted all files, including generated_plugin_registrant.dart. But on GitHub Actions, after generated_plugin_registrant.dart was generated, nothing was sorting its imports, so this tool was failing.

I hope I described it clearly and that nobody before had lost theirs time because of this bug:)

`generated_plugin_registrant.dart` file was not being ignored by import_sorter.
The logic was flawed - the slash was redundant.

* set VSCode line length for dartfmt to 80
if (dependencies.contains('flutter/') &&
dartFiles
.containsKey('${currentPath}/lib/generated_plugin_registrant.dart')) {
final containsFlutter = dependencies.contains('flutter');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR could basically be just changing flutter/ to flutter, but I decided to improve some other parts a bit as well.

.containsKey('${currentPath}/lib/generated_plugin_registrant.dart');

stdout.writeln('contains flutter: ${containsFlutter}');
stdout.writeln('contains registrant: ${containsRegistrant}');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to remove this after merge, I added this so it's more clear.

@@ -13,8 +10,9 @@ ImportSortData sortImports(
String package_name,
bool emojis,
bool exitIfChanged,
bool noComments,
) {
bool noComments, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional named argument so tests don't require refactors.

stdout.write('Please run import sorter!'.bold().red());
if (filePath != null) {
stdout.writeln(
'\n┗━━🚨 File ${filePath} does not have its imports sorted.');
Copy link
Contributor Author

@bartekpacia bartekpacia May 10, 2021

Choose a reason for hiding this comment

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

I've removed .bold().red() - believe me or not, but it doesn't show up on Ubuntu 20.04 LTS machines.

Also, printing the file name on which the tool failed is very useful.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented May 10, 2021

Another nice solution would be removing all logic related to removing generated_plugin_registrant.dart and just advising users to add it to ignored_files in pubspec.yaml.

@gleich gleich self-requested a review May 10, 2021 18:14
@gleich
Copy link
Member

gleich commented May 10, 2021

Hey @bartekpacia! Thank you so much for the PR! Sorry to hear that you were having an issue. Looks like you've added some pretty cool things here. I'll take a look after I finish school today :)

Copy link
Member

@gleich gleich left a comment

Choose a reason for hiding this comment

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

Hey @bartekpacia! Thanks for the work on this. Just a few little changes and then we should be good to merge.

Comment on lines 1 to 3
{
"dart.lineLength": 80
}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file

Copy link
Member

Choose a reason for hiding this comment

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

Actually see a comment I made later please

bin/main.dart Outdated
Comment on lines 99 to 105
file.readAsLinesSync(),
packageName,
emojis,
exitOnChange,
noComments,
filePath: filePath,
);
Copy link
Member

Choose a reason for hiding this comment

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

Is this from that vscode settings file? If so feel free to keep the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's me – I almost always (when it makes sense) add the trailing comma because it makes the code prettier :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the whole .vscode directory and add it to .gitignore, but then it'd be nice to include some info in README.md or CONTRIBUTING.md that this project uses lineLength = 80

Copy link
Member

Choose a reason for hiding this comment

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

That would be great, thank you!

stdout.write('Please run import sorter!'.bold().red());
if (filePath != null) {
stdout.writeln(
'\n┗━━🚨 File ${filePath} does not have its imports sorted.');
Copy link
Member

Choose a reason for hiding this comment

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

Please bring back the red and bold display. It's most likely a problem with your terminal's display colors/system if the color isn't being displayed correctly. It works fine for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't customize anything - just a default Ubuntu terminal (test it out for yourself – it's available on GitHub Actions). I think that many people are using Ubuntu and may encounter this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we can cut it then

@gleich gleich self-requested a review May 30, 2021 17:38
Copy link
Member

@gleich gleich left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for this!

@gleich gleich merged commit 9474753 into fluttercommunity:master May 30, 2021
@gleich
Copy link
Member

gleich commented May 30, 2021

This is now out in import_sorter v4.6.0. Thanks for the PR!

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