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

First attempt to add newlines between attributes. (#275) #731

Merged
merged 1 commit into from
May 25, 2024

Conversation

areleu
Copy link
Contributor

@areleu areleu commented Mar 28, 2024

Closes #275
This is an implementation of the suggestion here: #275 (comment)

It may be a little hacky, I am new to Rust so if I am doing some obvious mistake please tell me.

@areleu areleu changed the title First attempt to add newlines to attributes. (#275) First attempt to add newlines between attributes. (#275) Mar 28, 2024
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

Thanks for working on it. I think, that my initial proposal is simpler to use. To implement it we need a 3-state enum:

enum AttributeIndent {
  /// Initial state
  NoneAttributesWritten,
  /// Keep indent that should be used if `new_line()` would be called
  SomeAttributesWritten(usize),
  /// Write specified indent before writing attribute in `with_attribute()`
  Indent(usize),
}
// in new_line()
match state {
  NoneAttributesWritten => state = Indent(self.writer.indent.len()),
  SomeAttributesWritten(i) => state = Indent(i),
  Indent(_) => // write \n
}

// in with_attribute()
let i = match state {
  // neither .new_line() or .with_attribute() not yet called
  NoneAttributesWritten => self.writer.indent.len(),
  // .new_line() was not called, but .with_attribute() was
  SomeAttributesWritten(i) => i,
  Indent(i) => {
    // write \n + self.writer.indent + i
    i
  }
};
// write attribute
state = SomeAttributesWritten(i);

The resulting state diagram:

stateDiagram-v2
    [*] --> NoneAttributesWritten 
    NoneAttributesWritten --> SomeAttributesWritten1 : .with_attribute()
    NoneAttributesWritten --> Indent2 : .new_line()

    SomeAttributesWritten1 --> SomeAttributesWritten1 : .with_attribute()
    SomeAttributesWritten1 --> Indent1 : .new_line()

    Indent1 --> SomeAttributesWritten1 : .with_attribute()
    Indent1 --> Indent1 : .new_line()

    SomeAttributesWritten2 --> SomeAttributesWritten2 : .with_attribute()
    SomeAttributesWritten2 --> Indent2 : .new_line()

    Indent2 --> SomeAttributesWritten2 : .with_attribute()
    Indent2 --> Indent2 : .new_line()
Loading

src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
@areleu
Copy link
Contributor Author

areleu commented Mar 29, 2024

@Mingun Thanks a lot for the review, I implemented your suggestion. I hope I interpreted it properly. Let me know if you think it needs changes, otherwise I will write a couple of tests to look for edge cases.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 99.65986% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 61.90%. Comparing base (5d76174) to head (c8b427f).
Report is 11 commits behind head on master.

Files Patch % Lines
src/writer.rs 99.64% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   61.24%   61.90%   +0.66%     
==========================================
  Files          39       39              
  Lines       16277    16520     +243     
==========================================
+ Hits         9969    10227     +258     
+ Misses       6308     6293      -15     
Flag Coverage Δ
unittests 61.90% <99.65%> (+0.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

I didn't run code myself, but it seems to be correct. Please add other tests, update changelog.md and I think, it would be almost complete.

src/events/mod.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
@Mingun
Copy link
Collaborator

Mingun commented Mar 31, 2024

Thanks for you work!

I squashed all your commits, write tests in my standards (nested modules), add documentation.

I realized, that currently, because BytesStart::push_attribute is used, an extra space is inserted in front of the attribute. This is probably the reason why you originally put the state in BytesStart. In general, I have long planned to split event struct for events for reading and writing. I think, this is time to do that, or at least, introduce an internal BytesStart for writer where the code for writing attributes could be placed.

In any case, current writer code is not good, because:

  • it is not optimal, because it uses intermediate vector to store serialized attributes;
  • it does not escape attribute content, so you can put string " otherattr="vulnerable in it;
  • it does not allow you to choose between " and ' for attribute writing

For this reason, I want to hold onto a little bit of this PR to adapt my old code and then use its results here.

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

Ok, I found a way to do that without much refactoring.

@dralley, would you like to look? Otherwise it is good to me.

@Mingun
Copy link
Collaborator

Mingun commented Apr 1, 2024

Wait on merge it this, I found that tests are incomplete. In case of

<element first="1"
         second="2"/>

we always should use spaces, but now indent symbols are used. The fix is trivial, just need to add a buffer to ElementWriter with spaces (in order to not allocate it each time) and borrow from it when we want to write such indent. Also introduce a new variant to AttributeIndent to distinguish this indent from this situation:

<element
  first="1"
  second="2"/>

To test this situation it is enough adjust tests to use \t as indent symbol.

@areleu
Copy link
Contributor Author

areleu commented Apr 2, 2024

Wait on merge it this, I found that tests are incomplete. In case of

<element first="1"
         second="2"/>

we always should use spaces, but now indent symbols are used. The fix is trivial, just need to add a buffer to ElementWriter with spaces (in order to not allocate it each time) and borrow from it when we want to write such indent. Also introduce a new variant to AttributeIndent to distinguish this indent from this situation:

<element
  first="1"
  second="2"/>

To test this situation it is enough adjust tests to use \t as indent symbol.

What about implementing it in such a way that (1) only happens if the indent symbol is set to b otherwise (2) should happen. One could also calculate some sort of \t approximation for the latter, something that produces:

<element
   	first="1"
   	second="2"/>

I'm coming from the opinion that combining indentation symbols is not a good idea. (Even the rendering of \t in GitHub was different from what I was hoping, another example of why I would avoid mixing indentation symbols)

@Mingun
Copy link
Collaborator

Mingun commented Apr 2, 2024

Yes, I also think, that mixing indentation symbols is a bad idea, because some editors can easily break things by replacing spaces with tabs, but auto-selecting behaviour based on intendation settings seems to me even worse. The user can implement such behaviour yourself using our stupid plumbing interface.

@Mingun Mingun merged commit 740b788 into tafia:master May 25, 2024
6 checks passed
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.

Writer: Add ability to separate attributes by newlines
3 participants