Skip to content

feat: PointData Immutability #96

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

Merged
merged 8 commits into from
Jun 16, 2020
Merged

Conversation

bnayae
Copy link
Contributor

@bnayae bnayae commented Jun 4, 2020

Closes #93

Briefly describe your proposed changes:
Immutable PointData builder consider best practice especially when using RX & async operations.
It will prevent potential side effects.

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2020

Codecov Report

Merging #96 into master will decrease coverage by 0.30%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
- Coverage   16.15%   15.85%   -0.31%     
==========================================
  Files          49       49              
  Lines        4792     4883      +91     
  Branches      228      243      +15     
==========================================
  Hits          774      774              
- Misses       3998     4089      +91     
  Partials       20       20              
Impacted Files Coverage Δ
Client/Internal/MeasurementMapper.cs 0.00% <0.00%> (ø)
Client/Writes/PointData.cs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99181c8...8a6b193. Read the comment docs.

@bednar
Copy link
Contributor

bednar commented Jun 5, 2020

Thanks for PR!

@bednar bednar self-requested a review June 5, 2020 07:21
@bednar bednar added the enhancement New feature or request label Jun 5, 2020
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for this PR.

Could you please also update CHANGELOG.md?

@@ -215,25 +246,29 @@ public PointData Timestamp(DateTimeOffset timestamp, WritePrecision timeUnit)
/// <returns></returns>
public PointData Timestamp(Instant timestamp, WritePrecision timeUnit)
{
Precision = timeUnit;

BigInteger? time = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove a redundant initializer => BigInteger? time;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify which redundant did you mean?
is it line 249? it doesn't seem redundant to me because it is a local variable not _time which is immutable.

Copy link
Contributor

Choose a reason for hiding this comment

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

The assignment of null is unnecessary because is not used in any execution path.

var otherFields = other._fields;
result = result && _fields.Count == otherFields.Count &&
_fields.All(pair => otherFields.ContainsKey(pair.Key) &&
object.Equals(otherFields[pair.Key], pair.Value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove redundant qualifier => Equals(otherFields[pair.Key], pair.Value));

Copy link
Contributor

Choose a reason for hiding this comment

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

The object qualifier is unnecessary.

@bednar
Copy link
Contributor

bednar commented Jun 8, 2020

Hi @bnayae,

Could you revert a commit about move PointData to Client.Core (7af91a0). We want to focus on one thing in PR and this complicates review and merge this PR.

Anyway Good job and I am very thankful for these improvements of our client.

Thanks again.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

There are only code style glitches and We will be ready to merge.

@bnayae
Copy link
Contributor Author

bnayae commented Jun 10, 2020

Fixed all var issues

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Could you please revert 7af91a0 - "move PointData to Client.Core". We want to focus on one thing in PR. Please create a new PR for that.

I've prepared a patch for you:

chore_revert_move_PointData_to_Client_Core.txt

@bnayae
Copy link
Contributor Author

bnayae commented Jun 11, 2020

Could you please revert 7af91a0 - "move PointData to Client.Core". We want to focus on one thing in PR. Please create a new PR for that.

I've prepared a patch for you:

chore_revert_move_PointData_to_Client_Core.txt

How do I execute the patch?

@bednar
Copy link
Contributor

bednar commented Jun 12, 2020

Sorry @bnayae, the patch was generated by JetBrains Rider... and should be applied by Rider

Anyway here is a git compatible patch:
chore_revert_move_PointData_to_Client_Core-GIT.txt

You can apply it by:

 git am --signoff < chore_revert_move_PointData_to_Client_Core-GIT.txt

@bnayae bnayae requested a review from bednar June 15, 2020 13:39
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@bednar bednar added this to the 1.9.0 milestone Jun 16, 2020
@bednar bednar changed the title PointData Immutability feat: PointData Immutability Jun 16, 2020
@bednar bednar merged commit 5019a9f into influxdata:master Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contribution offer
3 participants