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 formatting for records #1

Merged
merged 16 commits into from
Nov 15, 2019
Merged

Add formatting for records #1

merged 16 commits into from
Nov 15, 2019

Conversation

folkertdev
Copy link
Contributor

currently, some tests are failing because

  • i'm not sure what the "correct" formatting is
  • and how to handle spaces (especially newlines)

Perhaps it's OK to ignore newlines exist for now, and always start with
{<space> and close with <space>}?

folkertdev and others added 4 commits November 13, 2019 22:10
currently, some tests are failing because

- i'm not sure what the "correct" formatting is
- and how to handle spaces (especially newlines)

Perhaps it's OK to ignore newlines for now, and just always start with
`{<space>` and close with `<space>}`?
@rtfeldman
Copy link
Contributor

Cool! I pushed some stuff:

  • Updated the format tests to the format I have in mind for records.
  • Added the beginnings of some is_multiline functions in src/fmt.rs. (We can probably move format from ast into the fmt module in the future, but no need to do that for this PR.) I implemented the one for expr all the way but left fields and pattern as TODO panics.
  • Made the spaces after the '{' and before the } work in the single-line case.

@rtfeldman
Copy link
Contributor

I've also been rethinking whether block comments are a good idea; I'm not sure I'll keep them in the language. (e.g. Python doesn't have them, so there's precedent for that being a reasonable design decision.)

The tests with the comments in unusual places are good though! I'd just change them to be line comments, so newlines are in the mix too.

src/parse/ast.rs Outdated
LabeledValue(name, spaces, value) => {
buf.push_str(name.value);

if spaces.len() > 0 {
Copy link
Contributor

@rtfeldman rtfeldman Nov 13, 2019

Choose a reason for hiding this comment

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

I think !spaces.is_empty() would be better here (and also below)

@folkertdev
Copy link
Contributor Author

I think the single-line case works correctly now. Multi-line requires a bit more work. I think in those cases newlines from the document should be ignored completely.

Also that logic is probably the same for records and lists? anyhow should multi-line formatting be part of this PR?

@rtfeldman rtfeldman merged commit 6c0e771 into trunk Nov 15, 2019
@rtfeldman rtfeldman deleted the format-records branch November 15, 2019 02:34
rtfeldman added a commit that referenced this pull request Dec 7, 2019
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