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.

Sign up for GitHub

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

Jump to bottom

Support --linefeed in Dart and Node #153

Merged
merged 11 commits into from
Jun 15, 2017
Merged

Support --linefeed in Dart and Node #153

merged 11 commits into from
Jun 15, 2017

Conversation

srawlins
Copy link
Contributor

@srawlins srawlins commented Jun 5, 2017

Addresses part of #8

If someone passes in something other than cr, crlf, lf, lfcr, it defaults of lf, like node-sass.

I'm happy to move parseLineFeed and enum LineFeed to... other files, open to suggestions.

I'm also happy to add tests, but emitsInOrder or whatever Stream, is not letting me see the line endings...

But here's a cmdline test, with the newlines marked with cat -e:

$ dart bin/sass.dart --linefeed cr bb.scss | cat -e
a {^M  b: 7;^M}$
$ dart bin/sass.dart --linefeed crlf bb.scss | cat -e
a {^M$
  b: 7;^M$
}$
$ dart bin/sass.dart --linefeed lf bb.scss | cat -e
a {$
  b: 7;$
}$

@nex3
Copy link
Contributor

nex3 commented Jun 5, 2017

Like with indentType and indentWidth, we're only supporting the linefeed parameter for compatibility with node-sass, so I'd prefer not to expose it as part of the Dart API or the CLI.

@srawlins
Copy link
Contributor Author

srawlins commented Jun 6, 2017

Done! Removed linefeed from Dart.

@nex3
Copy link
Contributor

nex3 commented Jun 6, 2017

Please also remove it from the command-line interface.

@srawlins
Copy link
Contributor Author

srawlins commented Jun 6, 2017

Done.

nex3
nex3 requested changes Jun 6, 2017
View reviewed changes
lib/src/node.dart Outdated
@@ -41,8 +41,11 @@ void _render(
var indentWidth = indentWidthValue is int
? indentWidthValue
: int.parse(indentWidthValue.toString());
var linefeed = parseLineFeed(options.linefeed);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consistently capitalize lineFeed, at least in places where we it doesn't need to be identical to the JS API. I think capitalizing "feed" is more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/src/render.dart Outdated
useSpaces: useSpaces, indentWidth: indentWidth, linefeed: linefeed);
}

LineFeed parseLineFeed(String str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should be a private method in node.dart.

Also, please document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/src/render.dart Outdated
case 'lfcr':
return LineFeed.LFCR;
break;
case 'lf':
Copy link
Contributor

Choose a reason for hiding this comment

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

This line probably isn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/src/render.dart Outdated
@@ -17,7 +16,8 @@ String render(String path,
{bool color: false,
SyncPackageResolver packageResolver,
bool useSpaces: true,
int indentWidth: 2}) {
int indentWidth: 2,
LineFeed linefeed: LineFeed.LF}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we avoid providing default argument values for non-boolean arguments, since it makes it impossible to explicitly say "use the default value". You can just use ?? in the method body instead.

Same goes for indentWidth, now that I think of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. and Done for indentWidth.

lib/src/visitor/serialize.dart Outdated
@@ -19,6 +19,15 @@ import 'interface/css.dart';
import 'interface/selector.dart';
import 'interface/value.dart';

enum LineFeed { CR, CRLF, LF, LFCR }
Copy link
Contributor

Choose a reason for hiding this comment

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

Official Dart enums suck 😝. If you use a custom class, you can just add the text as a field.

Also, I believe these should be all lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do ☹️ Done.

lib/src/visitor/serialize.dart Outdated
@@ -94,16 +105,20 @@ class _SerializeCssVisitor
/// The number of spaces or tabs to be used for indentation.
final int _indentWidth;

final String _linefeed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nex3 nex3 merged commit 7d92f2c into sass:master Jun 15, 2017
@nex3
Copy link
Contributor

nex3 commented Jun 15, 2017

Thanks Sam!

nex3 pushed a commit that referenced this pull request May 10, 2023
@jathak
Revert "Remove workaround for dart-lang/setup-dart#59 (#151)" (#153)
c6752b5 
This reverts commit 7d38d42.
jgerigmeyer added a commit to oddbird/dart-sass that referenced this pull request May 16, 2023
@jgerigmeyer
Merge branch 'main' into browser
8b68426 
* main: (162 commits)
  Move source and test files to namespaced subdirectories
  Update Dart Sass version and release
  Update Dart Sass version and release
  Update Dart Sass version and release
  Update Dart Sass version and release
  Update Dart Sass version and release
  Update Dart Sass version and release
  Revert "Remove workaround for dart-lang/setup-dart#59 (sass#151)" (sass#153)
  Update Dart Sass version and release
  Update Dart Sass version and release
  Remove workaround for dart-lang/setup-dart#59 (sass#151)
  Update Dart Sass version and release
  Update Dart Sass version and release
  Fix qemu releases (sass#149)
  Update Dart Sass version and release
  Bump sass_api from 4.2.1 to 5.0.0 (sass#143)
  Bump meta from 1.8.0 to 1.9.0 (sass#144)
  Bump grinder from 0.9.2 to 0.9.3 (sass#145)
  Add missing setup-dart step in qemu release (sass#147)
  Use buf instead of protoc to compile protobufs (sass#146)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nex3 nex3 nex3 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@srawlins @nex3