Skip to content

these are definitely weapons as well#167

Merged
VALERA771 merged 5 commits intoExMod-Team:scpsl14from
Mikihero:isweaponchange
Dec 9, 2024
Merged

these are definitely weapons as well#167
VALERA771 merged 5 commits intoExMod-Team:scpsl14from
Mikihero:isweaponchange

Conversation

@Mikihero
Copy link
Copy Markdown

@Mikihero Mikihero commented Nov 5, 2024

Description

Describe the changes

What is the current behavior? Item.IsWeapon only checks for firearms

public bool IsWeapon => this is Firearm;

What is the new behavior? Checks for Micro HID and jailbird as well as firearm

Does this PR introduce a breaking change? Technically yes, although I can't think of a use case where you wouldn't also want to include other weapons instead of just checking if it's a firearm.

Other information: As it technically is breaking, I was considering just making this a method (ideally a different property tho I culdn't think of a good name, hence a method of a similar name)


Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentations

Submission checklist

  • I have checked the project can be compiled
  • I have tested my changes and it worked as expected

Patches (if there are any changes related to Harmony patches)

  • I have checked no IL patching errors in the console

Other

  • Still requires more testing

@louis1706
Copy link
Copy Markdown

https://github.com/Mikihero/EXILED/blob/02daca40efaccf6974c7d85a7f5c977fa453e210/EXILED/Exiled.API/Extensions/ItemExtensions.cs#L42
I would use this for check if an item is an weapon this extension permit to check if it's firearm but yeah naming is kinda wrong

Copy link
Copy Markdown

@VALERA771 VALERA771 left a comment

Choose a reason for hiding this comment

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

I wanted to write a joke but I'm too lazy

@louis1706
Copy link
Copy Markdown

louis1706 commented Nov 9, 2024

in my opinion we should added IsFirearm

and put this one as Obsolete and for Exiled 9 doing this check with all possible Weapon

if you guys thinks it's better to do it directly it's your choice

@Mikihero
Copy link
Copy Markdown
Author

Mikihero commented Nov 9, 2024

in my opinion we should added IsFirearm

I guess it could be useful for people who have no idea how to cast but the actual code would be just bool IsFirearm => this is Firearm, aka what IsWeapon is rn

@louis1706
Copy link
Copy Markdown

in my opinion we should added IsFirearm

I guess it could be useful for people who have no idea how to cast but the actual code would be just bool IsFirearm => this is Firearm, aka what IsWeapon is rn

that was actually the only use case of this Propperty (they are more if you look under and above this one)

@Mikihero
Copy link
Copy Markdown
Author

in my opinion we should added IsFirearm

I guess it could be useful for people who have no idea how to cast but the actual code would be just bool IsFirearm => this is Firearm, aka what IsWeapon is rn

that was actually the only use case of this Propperty (they are more if you look under and above this one)

not sure what you mean

@github-actions github-actions bot removed the Events label Nov 18, 2024
@louis1706
Copy link
Copy Markdown

in my opinion we should added IsFirearm

I guess it could be useful for people who have no idea how to cast but the actual code would be just bool IsFirearm => this is Firearm, aka what IsWeapon is rn

that was actually the only use case of this Propperty (they are more if you look under and above this one)

not sure what you mean

I mean that it would be good to have a replacement for what they were using until now so we can tell them what to replace IsWeapon with IsFirearm

@Mikihero
Copy link
Copy Markdown
Author

makes sense, I can do that later

Copy link
Copy Markdown

@VALERA771 VALERA771 left a comment

Choose a reason for hiding this comment

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

I agree with yamato's suggestion. Also change branch to scpsl14 so we can merge your pr

Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
@Mikihero Mikihero changed the base branch from dev to scpsl14 December 4, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants