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

Add arguments and path, clean up #1

Merged
merged 5 commits into from Nov 29, 2019
Merged

Add arguments and path, clean up #1

merged 5 commits into from Nov 29, 2019

Conversation

Cretezy
Copy link
Contributor

@Cretezy Cretezy commented Nov 24, 2019

Added {} argument, with path. Also cleaned up some parts of the code (make final when can be) and the README (also added undocumented features such as . strings).

@Skyost Skyost self-assigned this Nov 26, 2019
@Skyost Skyost added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 26, 2019
@Skyost
Copy link
Owner

Skyost commented Nov 26, 2019

@Cretezy Okay so a few points :

  1. First, thanks a lot for contributing !
  2. You're totally right about the nested strings part.
  3. The second commit is also good but I prefer a strong typing in the project (and Dart allows us to do so).
  4. Why using a path argument with a getPathFunction ? It seems a bit duplicate to me as the default getPathFunction is exactly $path/language.json.
  5. One last thing : you've added three transitive dependencies in example/pubspec.lock : petitparser, image and xml. Are they needed ?
  6. I don't see the matter of putting final everywhere. There was a myth at the time of Java where everybody was saying that final was faster. It's clearly not the case in Dart 😄
  7. I think named arguments is a better and cleaner option to add arguments.

@Skyost
Copy link
Owner

Skyost commented Nov 26, 2019

I've made the following changes : 1e49dbd, is it fair to you ?

@Cretezy
Copy link
Contributor Author

Cretezy commented Nov 27, 2019

For 3, final does do strong typing since you can't redeclare the type.

For 4, it's usually easier to give a path than a function, so support for both is nicer.

For 5, pub did that on its own. Don't know why.

For 6, it's usually a best practice since it can lead to fewer bugs from accidentally assigning something.

(on mobile right now, will check again later)

@Skyost
Copy link
Owner

Skyost commented Nov 27, 2019

Yeah I understand your point of view. But anyway, I prefer not to make everything final and to replace it with the class name. That's a convention I've been using for a while now.

The path still seems duplicate to me. Maybe creating a function that allows to create a getPathFunction directly from a path ?

@Skyost Skyost merged commit c4a0453 into Skyost:master Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants