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

feat: check duplicated values #20

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ronnnnn
Copy link
Contributor

@ronnnnn ronnnnn commented Sep 5, 2023

Overview

Add check command and duplicates sub command to find duplicated values.

Use Case

Large arb file or many arb files make difficult to find duplicated values.
To find them easily, add the commands on arb_utils.

Review Points

  • Add duplicates command as sub command to make easy to add another commands on check command.
  • Add exit function when the command causes error to stop workflow on CI and make check not pass on GitHub.
  • To simplify, not check values which contain placeholders and keys which contain special attributes beginning from @@, at first.

@ronnnnn
Copy link
Contributor Author

ronnnnn commented Sep 5, 2023

@Rodsevich
Please review my ideas and codes.

@Rodsevich
Copy link
Owner

Hey @ronnnnn ! I'll happily do this today. Thanks for the PR!

Copy link
Owner

@Rodsevich Rodsevich left a comment

Choose a reason for hiding this comment

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

there are the changes I could barely see. Please make sure the English is corrected all over the code, I didn't manage to correct it on revery line

README.md Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
lib/src/commands/check_duplicate.dart Outdated Show resolved Hide resolved
@ronnnnn
Copy link
Contributor Author

ronnnnn commented Sep 6, 2023

@Rodsevich
Thank you for your reviews and quick response. I update codes and docs from your advices.

@ronnnnn ronnnnn changed the title feat: check duplicate values feat: check duplicated values Sep 7, 2023
Copy link
Owner

@Rodsevich Rodsevich left a comment

Choose a reason for hiding this comment

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

I'm thinking that taking the "check duplicates" command of the package just for values would be sortta bully, because somebody would eventually need to check keys/values/both (/documentations/@keywords/@@keywords/whatever) and he wouldn't be able to do so because the check duplicates oly checks for values. I think the best approach for solving that is creating subcommands on the duplicates subcommand and there add this logic for values or adding options to the duplicates subcommand to enable/disable what to check for, with just the values defaulted to true.
Note that if you want to add the other subcommands/options, you are welcome, but I'm just requesting for you the architectural support for subcommands over the duplicates subcommand, specifically the values. So, the client will launch the command arb_utils check duplicates values or arb_utils check duplicates --values for using your feature, leaving room for another user that would need to check for keys by using arb_utils check duplicates keys or arb_utils check duplicates --no-values --keys.

What do you think?


final file = File(filePaths.first);
if (!await file.exists()) {
print(red('ERROR! File ${file.path} does not exists'));
Copy link
Owner

Choose a reason for hiding this comment

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

Map<String, dynamic> checkDuplicatesARB(String arbContent) {
final Map<String, dynamic> arbJsonMap = json.decode(arbContent);
arbJsonMap.removeWhere((key, value) => key.startsWith('@@'));
arbJsonMap.removeWhere((key, value) => key.startsWith('@'));
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it'd be necessary to avoid the @keys

final Map<String, dynamic> arbJsonMap = json.decode(arbContent);
arbJsonMap.removeWhere((key, value) => key.startsWith('@@'));
arbJsonMap.removeWhere((key, value) => key.startsWith('@'));
arbJsonMap.removeWhere((key, value) {
Copy link
Owner

Choose a reason for hiding this comment

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

what's the intention of this code? write it down on a //comment

Are you sure the regexp works for every fomatting of the .arb? You'll have to format it before processing or improve the RegExp, maybe with \s\S

}
}
''';
const noDuplicatedArb = '''
Copy link
Owner

Choose a reason for hiding this comment

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

notDuplicatedArb*

import 'package:args/command_runner.dart';
import 'package:dcli/dcli.dart';

class CheckDuplicatesCommand extends Command {
Copy link
Owner

Choose a reason for hiding this comment

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

must be named CheckDuplicatesSubcommand

@ronnnnn
Copy link
Contributor Author

ronnnnn commented Sep 7, 2023

I agree with your approach.
I'll try to make foundational feature (supporting only duplicated values).
Thank you a lot for your advices.

@ronnnnn ronnnnn marked this pull request as draft September 19, 2023 00:53
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