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

dartfmt #166

Closed
beatgammit opened this issue Jan 4, 2015 · 8 comments
Closed

dartfmt #166

beatgammit opened this issue Jan 4, 2015 · 8 comments

Comments

@beatgammit
Copy link
Contributor

I ran dartfmt -w lib and got a 15,000 line diff. From what I can tell, most of the changes consist of:

  • spaces around contents in control flow statements, function arguments and indexers
    • if ( condition ) { -> if (condition) {
    • arr[ 1 ] = 3; -> arr[1] = 3;
  • indentation and alignment: tabs -> spaces, indent continuations
  • lines > 80 characters

The current style is pretty consistent and looks like it was taken from three.js. Should three.dart adopt the same style, or should it strive to be consistent with the Dart Style Guide? I think it should try to match the style guide, but if it doesn't, perhaps three.dart should have its own style guide.

There are also quite a few instances of for loops where a List.forEach would be cleaner.

This is mostly a question of whether three.dart should try to stick as close as possible to three.js or whether it intends to be Dart-like?

@nelsonsilva
Copy link
Member

Thank you for addressing this. The goal is to make three.dart as idiomatic as possible so we should strive to follow the style guide as much as possible.

As you guessed most of the incoherences come from the three.js styling but since we have dartfmt we should follow its rules (but with --line-length 120 at least... I've stopped programming on VTs some years ago :P)

Regarding the for => forEach I'm all for it (map, filter, reduce whenever you can ;))

One thing to keep in mind is that we'll have to arrange for these change to be merged only after closing relevant PR otherwise we'll find ourselves in rebasing/merging hell.

beatgammit added a commit to beatgammit/three.dart that referenced this issue Jan 6, 2015
robsilv added a commit that referenced this issue Jan 14, 2015
@beatgammit
Copy link
Contributor Author

@nelsonsilva The only PR left is yours, but it looks like there's only a minor modification to one file.

I'd like to move forward with this, but I'd like to know whether I should try to break it up into smaller commits, or just go for it all at once.

Thoughts?

@nelsonsilva
Copy link
Member

@beatgammit We can move forward without merging my PR. It's WIP and the PR is there just for tracking and discussion. GLTF is a moving target.

@beatgammit
Copy link
Contributor Author

@nelsonsilva - Should I break it up into smaller commits, or just do it all at once?

@nelsonsilva
Copy link
Member

I think this should be done all at once as breaking it up doesn't add any value to the history.
What do you guys think about using --line-length 120 ? @financecoding @robsilv @johsin18

@johsin18
Copy link
Contributor

I'm in favor of line length 120.

@eukreign
Copy link
Contributor

eukreign commented Feb 2, 2015

+1 for 120

Also, i'm looking forward to this update. I had to turn off "formatting on save" in Dart editor to make my last pull request not include a bunch of formatting changes. It will be nice when all of the code in three.dart is properly formatted and changing settings in dart editor is no longer necessary.

beatgammit added a commit to beatgammit/three.dart that referenced this issue Feb 3, 2015
@beatgammit
Copy link
Contributor Author

Ok, I have a pull-request ready, but it should probably wait until all pull-requests that don't just add files are resolved.

beatgammit added a commit to beatgammit/three.dart that referenced this issue Feb 4, 2015
nelsonsilva pushed a commit that referenced this issue Feb 4, 2015
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

No branches or pull requests

4 participants