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

Adding .editorconfig and applying naming conventions #26

Merged
merged 10 commits into from
Mar 2, 2023

Conversation

MichaelGHSeg
Copy link
Contributor

No description provided.

Add(new UserInfoPlugin());
_ = this.Add(new StartupQueue());
_ = this.Add(new ContextPlugin());
_ = this.Add(new UserInfoPlugin());
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably should just ignore the return values. this looks bad imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nuked

userInfo.traits = null;
this._userInfo._userId = null;
this._userInfo._anonymousId = null;
this._userInfo._traits = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot about this last time. it's probably better that we just create a new UserInfo here, instead of assigning the properties one by one. also, assigning anonymousId as null actually introduce a bug, since the ResetAction below actually gives a random uuid, which creates a inconsistent image between memory and sovran.
refer to the kotlin counterpart here for solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Analytics-CSharp/Segment/Analytics/Configuration.cs Outdated Show resolved Hide resolved
public JsonObject integrations;
public JsonObject plan;
public JsonObject edgeFunctions;
#pragma warning disable IDE1006 // Naming Styles
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this pragma? is it for disable warnings from the IDE? if so, we don't have to have it in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed


System(Configuration configuration, Settings settings, bool running)
private System(Configuration configuration, Settings settings, bool running)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be private. it's possible that we create a System state directly instead of through DefaultState. we don't need to apply every IDE suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


UserInfo(string anonymousId, string userId, JsonObject traits)
private UserInfo(string anonymousId, string userId, JsonObject traits)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


// backing fields that holds the actual string representation
// needed for switch statement, has to be compile time available
#pragma warning disable IDE1006
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

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

Looks great!! 🔥

@MichaelGHSeg MichaelGHSeg merged commit e71925b into main Mar 2, 2023
@MichaelGHSeg MichaelGHSeg deleted the MichaelGHSeg/LIBMOBILE-1095 branch March 2, 2023 23:47
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