Skip to content

Conversation

@KyleSchoonover
Copy link
Contributor

Jira

Tests

  • My PR does not need testing for this extremely good reason: non functional change

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@github-actions github-actions bot added the C# label Feb 1, 2022
(byte)'b',
(byte)'j',
Version };
public static readonly byte[] Magic =
Copy link
Member

Choose a reason for hiding this comment

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

The looks like an API change but I guess no one is supposed to change the magic bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change was formatting.

Copy link
Member

Choose a reason for hiding this comment

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

The change was formatting.

What do you mean ?

AFAIU readonly makes Magic a constant, like final in Java.
Until now a user can change the value of Magic in his/her application. After this PR it won't be possible anymore. And such application will fail to compile. This is what I meant by API change.
But I agree that no one is supposed to change the magic bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. Do we want the backwards support to allow the change? It appears the intention was that it is a constant hence the readonly static. Like with most things, if it is available someone is using it. The other option is I can mark this obsolete, and add the remark for a future change that it be made readonly.

Copy link
Member

Choose a reason for hiding this comment

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

I am 99% sure no one is setting custom value for the Magic field. So I am OK to merge it as is! But only for 1.12.0!
If someone complains that we broke his/her build then we can revert if (s)he gives us a really convincing reason!

/// </summary>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Design",
"CA1052:Static holder types should be Static or NotInheritable",
Justification = "Maintain public API")]
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to the Magic constant ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, but it's an old rule. The way you get rid of it, is just make the static property readonly. Allows you to essentially make a static property a constant. The reason you do this is because constants are basic types and not a reference type like an array.

@RyanSkraba RyanSkraba changed the title AVRO-3345 Made Magic readonly in DataFileConstants AVRO-3345: Made Magic readonly in DataFileConstants Feb 1, 2022
@martin-g martin-g merged commit 18d7495 into apache:master Feb 1, 2022
@KyleSchoonover KyleSchoonover deleted the AVRO-3345 branch February 3, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants