Skip to content

Conversation

Sanlovty
Copy link
Contributor

@Sanlovty Sanlovty commented Jun 29, 2022

Description of the Change

Add a subgroup system to allow the user to create one class that includes different asset groups with unique file prefixes, paths, and extensions.

Benefits

Now you can create more flexible aseet-classes by providing different paths, file extensions and prefixes into the same class:

Config:

  generate_tests: true
  no_comments: true
  export: true
  use_part_of: true
  use_references_list: false
  package: resources
  groups:
    - class_name: Assets
      subgroups:
        - path: assets/images
          types: [ jpg ]
          prefix: jpg
        - paths: 
          - assets/images
          - assets/imgs
          types: [ png ]
          prefix: png

Result:

part of 'resources.dart';

class Assets{
  Assets ._();
  
  static const String jpgTest2 = 'assets/images/test2.jpg';
  static const String pngTest1 = 'assets/images/test1.png';
  static const String pngTest3 = 'assets/imgs/test3.png';
}

Possible Drawbacks

  • deeper (complicated) structure of config file.
  • not the best name ('subgroups').

Verification Process

What process did you follow to verify that your change has the desired effects?

  • test adaptation.
  • validateConfigs adaptation.

Applicable Issues

#49

@BirjuVachhani BirjuVachhani added the feature indicates that a feature is requested label Jun 30, 2022
@BirjuVachhani
Copy link
Owner

@Sanlovty Let's not add a dependency to tuple if you're only using Tuple2. We can have a local class model that makes more sense!

@Sanlovty
Copy link
Contributor Author

Sanlovty commented Jun 30, 2022

@Sanlovty Let's not add a dependency to tuple if you're only using Tuple2. We can have a local class model that makes more sense!

Sure, i'll make it <3

@Sanlovty
Copy link
Contributor Author

@BirjuVachhani, can u assign me to pr?

@BirjuVachhani
Copy link
Owner

@Sanlovty When I said Replace tuple with something more meaningful I meant something like this!

class SubgroupProperty {
  final String prefix;
  final Map<String, String> files;
}

Its fine if you don't have time for now to do this or if you don't want to. I can always take a look later.

@Sanlovty
Copy link
Contributor Author

Its fine if you don't have time for now to do this or if you don't want to. I can always take a look later.

Oh, my bad. I'll fix it. I have time and i want to do it.

Sanlovty added 7 commits June 30, 2022 16:10
- add optional types
- add optional paths
- add optional prefix
- make subgroups optional
- refactor fromJson method

The properties (types,prefix) of the group are now overriding the sub_groups' properties. If a group has a path\paths property, it simply does not use a sub_groups at all due to their futility
@BirjuVachhani
Copy link
Owner

@Sanlovty Thank you! Appreciate that! 😊

@Sanlovty
Copy link
Contributor Author

Sanlovty commented Jun 30, 2022

@BirjuVachhani, I think it’s working as you want now. I want to add effective tests to touch each case, but I also need to rewrite some of the existing ones. I’m waiting for your feedback.

Example of using:

generate_tests: true
no_comments: true
export: true
use_part_of: true
use_references_list: false
package: resources

groups:
  - class_name: Images
    path: assets/images
    types: [ .png, .jpg, .jpeg, .webp, .webm, .bmp ]
  
  - class_name: Svgs
    sub_groups:
      - path: assets/svgsMenu
        prefix: menu
        types: [ .svg ]

      - path: assets/svgsOther
        prefix: other
        types: [ .svg ]
  
  - class_name: Ico
    types: [ .ico ]
    prefix: ico
    sub_groups:
      - path: assets/icons
        prefix: test1
        types: [ .ttf ]

      - path: assets/vectors
        prefix: test2
        types: [ .pdf ]
  
  - class_name: Video
    types: [ .mp4 ]
    path: assets/moviesOnly
    sub_groups:
      - path: assets/movies
        prefix: common

      - path: assets/moviesExtra
        prefix: extra

I am online from 7:00 to 22:00 (GMT)

Comment on lines +42 to +73
- class_name: Svgs
sub_groups:
- path: assets/svgsMenu
prefix: menu
types: [ .svg ]
- path: assets/svgsOther
prefix: other
types: [ .svg ]
- class_name: Ico
types: [ .ico ]
prefix: ico
sub_groups:
- path: assets/icons
prefix: test1
types: [ .ttf ]
- path: assets/vectors
prefix: test2
types: [ .pdf ]
- class_name: Video
types: [ .mp4 ]
path: assets/moviesOnly
sub_groups:
- path: assets/movies
prefix: common
- path: assets/moviesExtra
prefix: extra
Copy link
Owner

Choose a reason for hiding this comment

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

Lets not change default configs. Users mostly don't do this! We'll document this at https://birjuvachhani.github.io/spider

Suggested change
- class_name: Svgs
sub_groups:
- path: assets/svgsMenu
prefix: menu
types: [ .svg ]
- path: assets/svgsOther
prefix: other
types: [ .svg ]
- class_name: Ico
types: [ .ico ]
prefix: ico
sub_groups:
- path: assets/icons
prefix: test1
types: [ .ttf ]
- path: assets/vectors
prefix: test2
types: [ .pdf ]
- class_name: Video
types: [ .mp4 ]
path: assets/moviesOnly
sub_groups:
- path: assets/movies
prefix: common
- path: assets/moviesExtra
prefix: extra

Copy link
Contributor Author

@Sanlovty Sanlovty Jun 30, 2022

Choose a reason for hiding this comment

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

This config is used in createConfigs method. I need to change it to run the tests

Copy link
Owner

Choose a reason for hiding this comment

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

If you only require this in tests then maybe we can change some things there to have it for tests only. I'd still prefer to not have all this when you actually run spider create command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, i'll rework the createConfig method

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you!

@BirjuVachhani
Copy link
Owner

@Sanlovty Over all look good. Nice work! 👍🏻 Just some minor changes and tests, other than that, it should be good to merge.

@Sanlovty
Copy link
Contributor Author

Sanlovty commented Jul 1, 2022

@BirjuVachhani if u agree with commits after cd55a39, than i start making the tests. Also, pls check email ^-^

I will appreciate if u suggest the naming ideas for vars\getters. I also was thinking about not making testConfigs 'getters' but we easily can change it if needed

@BirjuVachhani
Copy link
Owner

@Sanlovty Those commits look good. I like names you have given to var/getters.

I also was thinking about not making testConfigs 'getters' but we easily can change it if needed

Agreed. I am fine either way.

Repository owner deleted a comment from codecov-commenter Jul 2, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #50 (a8a1887) into main (4e4cc97) will increase coverage by 2.26%.
The diff coverage is 89.43%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   68.30%   70.56%   +2.26%     
==========================================
  Files          13       15       +2     
  Lines         489      564      +75     
==========================================
+ Hits          334      398      +64     
- Misses        155      166      +11     
Impacted Files Coverage Δ
lib/src/constants.dart 50.00% <ø> (ø)
lib/spider.dart 40.62% <33.33%> (+0.94%) ⬆️
lib/src/asset_subgroup.dart 81.81% <81.81%> (ø)
lib/src/asset_group.dart 83.87% <86.95%> (+0.53%) ⬆️
lib/src/dart_class_generator.dart 91.87% <92.10%> (-2.57%) ⬇️
lib/src/utils.dart 78.74% <92.30%> (+1.54%) ⬆️
lib/src/data/test_template.dart 80.00% <100.00%> (+13.33%) ⬆️
lib/src/subgroup_property.dart 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 537e608...a8a1887. Read the comment docs.

@Sanlovty Sanlovty requested a review from BirjuVachhani July 6, 2022 14:37
Copy link
Owner

@BirjuVachhani BirjuVachhani left a comment

Choose a reason for hiding this comment

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

LGTM

@BirjuVachhani BirjuVachhani merged commit 09665c8 into BirjuVachhani:main Jul 10, 2022
@BirjuVachhani
Copy link
Owner

Thank you @Sanlovty for your contributions. 😊

@BirjuVachhani
Copy link
Owner

Released in v3.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature indicates that a feature is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants