Skip to content
This repository has been archived by the owner on Apr 17, 2021. It is now read-only.

Corrupted implicit on jewel missing after parsing/not being parsed #65

Closed
Eruyome opened this issue Feb 11, 2017 · 8 comments
Closed

Corrupted implicit on jewel missing after parsing/not being parsed #65

Eruyome opened this issue Feb 11, 2017 · 8 comments

Comments

@Eruyome
Copy link
Collaborator

Eruyome commented Feb 11, 2017

Haven't looked into it, just making a note for now. Tested with latest TradeMacro code and also current ItemInfo master code.

unbenannt

Rarity: Rare
Vivid Stone
Viridian Jewel
--------
Item Level: 70
--------
3% additional Chance to receive a Critical Strike
--------
15% increased Physical Damage with Daggers
9% increased Chaos Damage
+10% to all Elemental Resistances
--------
Place into an allocated Jewel Socket on the Passive Skill Tree. Right click to remove from the Socket.
--------
Corrupted
@Eruyome
Copy link
Collaborator Author

Eruyome commented Feb 11, 2017

Question @aRTy42 what item types are there that don't have implicit mods unless corrupted? Only jewels? This is the problem:


; Retrieve items implicit mod if it has one
	If (Item.IsWeapon or Item.IsArmour or Item.IsRing or Item.IsBelt or Item.IsAmulet) {

@Eruyome
Copy link
Collaborator Author

Eruyome commented Feb 11, 2017

Should be fixed (unless there are more item types that aren't included).

@Eruyome Eruyome closed this as completed Feb 11, 2017
@aRTy42
Copy link
Owner

aRTy42 commented Feb 11, 2017

Yes, I think only jewels are missing there.

@Eruyome
Copy link
Collaborator Author

Eruyome commented Feb 11, 2017

Btw, I'm currently improving the update function to allow :

x.x.x-alpha.x
x.x.x-beta.x
x.x.x-rc.x

Where rc > beta > alpha, should a release candidate be higher/newer than a beta?

@aRTy42
Copy link
Owner

aRTy42 commented Feb 11, 2017

I think that is fine, but I have practically no expertise in that regard.

@Eruyome
Copy link
Collaborator Author

Eruyome commented Feb 11, 2017

I don't have that, too, just took a quick look at semver.org but looking at bit more I just found this:

A larger set of pre-release fields has a higher precedence than a smaller set, if all of the preceding identifiers are equal. Example: 1.0.0-alpha < 1.0.0-alpha.1 < 1.0.0-alpha.beta < 1.0.0-beta < 1.0.0-beta.2 < 1.0.0-beta.11 < 1.0.0-rc.1 < 1.0.0.

Btw, according to semantic versioning you're doing it wrong, there shouldn't be a leading zero anywhere 2.5.05 ;-)

And you're incrementing the patch version too much while the minor version not often enough
MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards-compatible manner, and PATCH version when you make backwards-compatible bug fixes.

Not that I care much about it though.

@aRTy42
Copy link
Owner

aRTy42 commented Feb 11, 2017

I'm using the version number to indicate for/during which PoE version it was released. PoE 2.5 gets the 2.5.x versions.

Changing the version number depending on the kind of update is a reasonable thing, but judging from what I read are people updating their script so differently and randomly that I doubt that it makes any difference. In particular now with the update notifier people will likely not look for the version number and decide based on that whether to update or not. Also semver.org is about software with an API, so the whole point of the strict definitions when to increase which version number part is done to deal with API/software conflicts. This seems not really applicable for ItemInfo.

I was using a leading zero because sorting seems more reliable that way, as some systems/environments use 1 < 11...19 < 2 < 21...29 < 3 etc. The updater was fine with it, wasn't it? If it is only a recommendation by the GitHub cofounder guy there, I'd rather stick to leading zeros.

@Eruyome
Copy link
Collaborator Author

Eruyome commented Feb 11, 2017

I think its less about having an API or not.

The updater doesn't have a problem with it and if it does it's easy to remove that leading zero for comparison purposes. There's no good reason to change your versioning logic aside from using a more standard approach. I actually didn't know that yours is related to the PoE version. Emmit does the same and it's not a bad idea in general.

The usual method I have seen is X.Y.Z, which generally corresponds to major.minor.patch:

Major version numbers change whenever there is some significant change being introduced. For example, a large or potentially backward-incompatible change to a software package.
Minor version numbers change when a new, minor feature is introduced or when a set of smaller features is rolled out.
Patch numbers change when a new build of the software is released to customers. This is normally for small bug-fixes or the like.

I actually made a breaking change to the macro once without realising it and changing the version accordingly, it was when we renamed a lot of files/folders and I have a few datafiles urls hardcoded to download them from the repo. Older versions didn't work anymore.

Eruyome added a commit that referenced this issue Mar 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants