fluttercommunity / community

Flutter Community - A central place for community made Flutter content.
1.58k stars 121 forks source link

Package Proposal: import_sorter #49

Closed gleich closed 4 years ago

gleich commented 4 years ago

Package Proposal: import_sorter

Dependency name (as used in pubspec.yaml): import_sorter Current pub.dev link: https://pub.dev/packages/import_sorter Current Git repository link: https://github.com/Matt-Gleich/import_sorter Description: Dart package that automatically sorts all your flutter or dart imports Current maintainer: Matthew Gleich, matthewgleich@gmail.com, @Matt-Gleich Needs new maintainer after transfer: NO Reason for transfer: Wanna expose more people to this package as I think it can help a lot of people. Comments: Excited πŸ˜„

jeroen-meijer commented 4 years ago

Awesome!

This is really cool. Especially love the emoji support 😁

To do before approval

As soon as these issues are fixed, the package will get rereviewed and added to the Flutter Community.

  1. Incompatible with transitive dependencies. As an example; in Flutter, you can use @required and do import 'package:meta/meta.dart'; without having meta in your dependencies, because meta is a transitive dependency of flutter. However, running the tool with this condition removes all meta imports and creates errors. I was doubting whether this is an actual issue, but transitive dependencies are commonly used, so this tool removing them can be a reason for people to uninstall it (which I wouldn't want because this packages is super sweet).
  2. Unexpected exception when formatting a certain project. I tried this tool on a regular Flutter project (with a similar structure to this project). When running the tool on this project, I get the following error:
    $ flutter pub run import_sorter:main
    βœ… Formatted /lib/app/assets.dart
    βœ… Formatted /lib/app/context.dart
    βœ… Formatted /lib/app/app.dart
    βœ… Formatted /lib/app/theme.dart
    βœ… Formatted /lib/utils.dart
    βœ… Formatted /lib/backend/app_state_store.dart
    βœ… Formatted /lib/backend/backend.dart
    βœ… Formatted /lib/backend/utils/observable.dart
    βœ… Formatted /lib/backend/utils/behaviour_subject.dart
    Unhandled exception:
    RangeError (index): Invalid value: Not in range 0..11, inclusive: 12
    #0      List.[] (dart:core-patch/growable_array.dart:146:60)
    #1      sortImports (package:import_sorter/sort.dart:35:14)
    #2      main (file:///Users/jeroen/Development/flutter/.pub-cache/hosted/pub.dartlang.org/import_sorter-1.0.7/bin/main.dart:41:38)
    #3      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:299:32)
    #4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)
    pub finished with exit code 255

    Unfortunately, this project is currently not open source, so I can't give you the code to test it. However, I'll try to see if I can isolate the issue. Hope this error log gives you any clues. Let me know if you want more info.


Please make these changes and tell me when you've done so in this issue. If you have any questions, don't hesitate to ask. πŸ‘πŸ»

Best regards.

gleich commented 4 years ago

Thanks! I'll let you know when I've implemented the changes.

gleich commented 4 years ago

Just resolved the issue with transitive dependencies. I now get the list of all the dependencies from pubspec.lock and it even runs flutter pub get or pub get automatically to ensure the pubspec.lock is up to date!

As far as the error you are getting goes I have no idea why. I tried running it with a similar project and got no error. What it looks like is happening is I am trying to get the value at i + 1 in a for loop. This shouldn't be a problem though because before that I check in an if statement to see if i != lines.length. Here is the code snippet surround this:

for (int i = 0; i < lines.length; i++) {
    // If line is an import line
    if (lines[i].startsWith('import ') && lines[i].endsWith(';')) {
      if (lines[i].contains('dart:')) {
        dartImports.add(lines[i]);
      } else if (lines[i].contains('package:flutter/')) {
        flutterImports.add(lines[i]);
      } else if (lines[i].contains('package:$package_name/') ||
          !lines[i].contains('package:')) {
        projectImports.add(lines[i]);
      }
      for (final dependency in dependencies) {
        if (lines[i].contains('package:$dependency/') &&
            dependency != 'flutter') {
          packageImports.add(lines[i]);
        }
      }
    } else if (i != lines.length &&
        lines[i].contains('//') &&
        lines[i + 1].startsWith('import ') &&
        lines[i + 1].endsWith(';')) {
    } else if (dartImports.isEmpty &&
        flutterImports.isEmpty &&
        packageImports.isEmpty &&
        projectImports.isEmpty) {
      beforeImportLines.add(lines[i]);
    } else {
      afterImportLines.add(lines[i]);
    }
  }
jeroen-meijer commented 4 years ago

Great work!

I have good news, I found the culprit. πŸ˜„

I have a completely commented out class in that project.

// import 'package:cloud_firestore/cloud_firestore.dart';
// import 'package:json_annotation/json_annotation.dart';

// class TimestampConverter implements JsonConverter<DateTime, Timestamp> {
//   const TimestampConverter();

//   @override
//   DateTime fromJson(Timestamp value) => value.toDate();

//   @override
//   Timestamp toJson(DateTime value) => Timestamp.fromDate(value);
// }

If I remove that, it all formats properly. I hope that fact and the stack trace I gave you helps resolve the error. Let me know if I can do anything else! πŸ‘‹πŸ»

gleich commented 4 years ago

Oh cool. Is there anything else I need to do to move forward with the approval process? Thanks!

jeroen-meijer commented 4 years ago

The only thing to do is fix the above error, so make sure commented files don't make the tool crash 😊

gleich commented 4 years ago

Sounds good! Should be done soon

gleich commented 4 years ago

Finished! 1.0.9 fixes the comment issue.

jeroen-meijer commented 4 years ago

LGTM

Approved

Thanks for submitting this package. It has been reviewed and approved for release on the Flutter Community GitHub! πŸŽ‰ Great work. πŸ‘πŸ»

What to do next

Please follow steps 3 through 7 in the Flutter Community Transfer Guide and let me know when you're done.

Good luck! πŸ‘‹πŸ»

gleich commented 4 years ago

All ready!

jeroen-meijer commented 4 years ago

Great!

Transfer ownership on pub.dev

You've just been added as a Flutter Community pub.dev publisher member. Please check your email.

Accept the invite, then go to your package and transfer ownership to fluttercommunity.dev

image

Transfer the package to the organization

You have also been invited to be a member of the Flutter Community GitHub organization. After accepting your invitation, please go to your repository's settings and transfer the package to the user/organization fluttercommunity.

image

Let me know if you have any questions or need help! πŸ‘πŸ»

gleich commented 4 years ago

I am getting a You don’t have the permission to create public repositories on fluttercommunity error when trying to transfer the repo.

jeroen-meijer commented 4 years ago

Sorry about that! We recently changed our member permission system. Please try again.

gleich commented 4 years ago

Done! For some reason, I am not able to change the description of the repo. I saw that some other repos had Maintainer: @Matt-Gleich in their description. Can you please enable that for me? Thanks!

jeroen-meijer commented 4 years ago

Awesome. That's the next step! Let me fix that for you.

jeroen-meijer commented 4 years ago

Done! You should have access to maintain anything on the repo now.

Let me know if you have any remaining questions. Thanks for the package! πŸ‘‹πŸ»

Closing this.

gleich commented 4 years ago

Thanks!

jeroen-meijer commented 4 years ago

PS: Your package will show up on our package table soon. πŸ‘πŸ»

jeroen-meijer commented 4 years ago
image

Tada~ πŸŽ‰