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

refactor: package event listener #177

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

rYuuk
Copy link
Contributor

@rYuuk rYuuk commented Dec 1, 2023

Description

  • The model namespace caused PackageCoreInfo.cs to no come in same asmdef, hence causing error when it was used outside of this asmdef.
  • I made it simpler by removing redundant code and using simple string type.

@rYuuk rYuuk self-assigned this Dec 1, 2023
@rYuuk rYuuk requested a review from a team as a code owner December 1, 2023 06:13
[InitializeOnLoadMethod]
static void Initialize()
{
Events.registeringPackages += OnPackagesInstalled;
Events.registeredPackages += OnPackagesInstalled;
Copy link
Contributor

Choose a reason for hiding this comment

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

@HarrisonHough HarrisonHough self-requested a review December 1, 2023 12:01
Copy link
Collaborator

@HarrisonHough HarrisonHough left a comment

Choose a reason for hiding this comment

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

Code looks fine and seems to work. I mentioned this to you already but I thought that it also worked before with the initial implementation, at least when I remember tested 😅. I remember complaining to Max about firing multiple events but then I realized it was because of the multiple packages or whatever

It was hard to find the events in Amplitude seems there is some delay with the sync or something

@MaxAndreassenRPM
Copy link
Contributor

MaxAndreassenRPM commented Dec 1, 2023

Code looks fine and seems to work. I mentioned this to you already but I thought that it also worked before with the initial implementation, at least when I remember tested 😅. I remember complaining to Max about firing multiple events but then I realized it was because of the multiple packages or whatever

Yup, I remember the same - I spoke to @rYuuk about it and we were trying to understand if there was something about his local setup that prevented the other event from working for him, because I also went back to look at past Amplitude events and it definitely was also working previously too.

@HarrisonHough
Copy link
Collaborator

HarrisonHough commented Dec 1, 2023

Code looks fine and seems to work. I mentioned this to you already but I thought that it also worked before with the initial implementation, at least when I remember tested 😅. I remember complaining to Max about firing multiple events but then I realized it was because of the multiple packages or whatever

Yup, I remember the same - I spoke to @rYuuk about it and we were trying to understand if there was something about his local setup that prevented the other event from working for him, because I also went back to look at past Amplitude events and it definitely was also working previously too.

not sure if relevant here but when calling stuff in editor via [InitializeOnLoadMethod] I find that behaviour for order of operations can differ per machine. In particular I notice the order is different on a mac. But there are also some errors or logs that show in different order depending on whether I use my personal laptop or work one 😅

@rYuuk rYuuk merged commit bfdc1af into develop Dec 4, 2023
@rYuuk rYuuk deleted the feature/refactor-package-event-listener branch December 4, 2023 06:17
HarrisonHough added a commit that referenced this pull request Dec 4, 2023
- The model namespace caused PackageCoreInfo.cs to not come in same
asmdef, hence causing error when it was used outside of this asmdef.
- I made it simpler by removing redundant code and using simple string
type.
HarrisonHough pushed a commit that referenced this pull request Dec 4, 2023
## Description

- The model namespace caused PackageCoreInfo.cs to no come in same
asmdef, hence causing error when it was used outside of this asmdef.
- I made it simpler by removing redundant code and using simple string
type.
HarrisonHough pushed a commit that referenced this pull request Dec 4, 2023
## Description

- The model namespace caused PackageCoreInfo.cs to no come in same
asmdef, hence causing error when it was used outside of this asmdef.
- I made it simpler by removing redundant code and using simple string
type.
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