Skip to content

Conversation

@Mingun
Copy link
Collaborator

@Mingun Mingun commented Aug 10, 2021

  • Fix warnings from Tracking Issue for denying trailing semicolons in expression macro bodies rust-lang/rust#79813:
    warning: trailing semicolon in macro used in expression position
      --> src\events\attributes.rs:362:20
        |
    362 |                 }));
        |                    ^
    ...
    443 |             None => attr!(start_key..end_key),
        |                     ------------------------- in this macro invocation
        |
        = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
        = note: for more information, see issue #79813 <https://github.com/rust-lang/rust/issues/79813>
        = note: macro invocations at the end of a block are treated as expressions
        = note: to ignore the value produced by the macro, add a semicolon after the invocation of `attr`
        = note: this warning originates in the macro `attr` (in Nightly builds, run with -Z macro-backtrace for more info)
    Because in the end it's stay non-obvilious that macro return some value, I've moved out the return keyword from the macro.
  • Fix a bug introduced in the 8481c44 -- some errors do not propagated to the caller
  • Remove unused import
  • Some bit of documentation in the deserializer
  • Remove excess cfg check -- it was already checked couple lines above.

@dralley
Copy link
Collaborator

dralley commented Aug 10, 2021

Could you also fix the CI which is apparently silently broken?

@dralley
Copy link
Collaborator

dralley commented Aug 10, 2021

https://github.com/tafia/quick-xml/blob/master/.github/workflows/rust.yml#L18

This

    - name: Run tests (no features)
      run: cargo test
      run: cargo test --no-default-features

needs to become

    - name: Run tests (no features)
      run: |
          cargo test
          cargo test --no-default-features

@dralley
Copy link
Collaborator

dralley commented Aug 10, 2021

Notice the green check mark on this PR even though the tests fail to run, I'm pretty sure that's what happened with the other PR as well.

@Mingun
Copy link
Collaborator Author

Mingun commented Aug 10, 2021

needs to become

Are you sure? It seems that line

run: cargo test

need to be completely removed

@dralley
Copy link
Collaborator

dralley commented Aug 10, 2021

Sure, that's possible too. Maybe the duplicate line was introduced as part of a bad merge conflict resolution. I'm only speaking about what needs to be done to make it parse properly

@tafia
Copy link
Owner

tafia commented Aug 11, 2021

Thanks! Indeed I blindly followed the green mark on the PR. Seems to be better now.
@Mingun do you mind fixing the last error?

@Mingun
Copy link
Collaborator Author

Mingun commented Aug 11, 2021

Yeah, need always to test changes themself... and do not forget to add --all-features 😄

Fixed the last error.

@tafia tafia merged commit 2dbb753 into tafia:master Aug 12, 2021
@tafia
Copy link
Owner

tafia commented Aug 12, 2021

Awesome thanks!

@Mingun Mingun deleted the fix-warnings branch August 12, 2021 04:31
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.

3 participants