Improve EnumInteger-System #41

JaniruTEC · 2020-05-24T13:52:45Z

Added new package conv (conversion)
Added convenience methods
Added missing static methods
Added IntegerConvertible as base interface of EnumInteger
Added MaskValue as special case of EnumInteger that is used to express masks
Renamed EnumIntegerSet to MaskValueSet and changed it to only accept MaskValues
Made EnumIntegerSet an interface and added EnumIntegerSetImpl as implementation
Renamed enumSetFromInt to ~~getSetFromInt~~ maskValueSet

Moved EnumInteger to new package conv (conversion) as first step to refactoring the EnumInteger-System Moved the appropiate Test Updated imports in all accessors

Moved EnumIntegerSet to package conv Moved the appropiate Test Updated imports in all accessors

Generified EnumInteger to not necessarily represent a bitmask, but an intValue that represents the Enum Modified depended classes to reflect this change Modified Tests to reflect this change Added overloaded version of EnumInteger#enumFromInt that accepts an enum-type instead of an array of possible values

…o an EnumIntegerSet

Renamed EnumIntegerSet to MaskValueSet Updated depended classes Renamed appropriate Test

Changed MaskValueSet to only accept MaskValueEnums (and therefore match it's contract)

Changed MaskValueSet to an interface and added class MaskValueSetImpl as implementor

…hod and renamed #fromInt Renamed MaskValueSet#enumSetFromInt to #getSetFromInt Renamed #fromInt in MaskValueEnums to #getSetFromInt Added #getSetFromInt to MaskValueEnums where it was missing Updated depended classes Added overloaded version of MaskValueSet#getSetFromInt that accepts an enum-type instead of an array of possible values Added delegate-method (getSetFromInt) to MaskValueEnum

Added #fromInt to EnumIntegers where it was missing

…egers

infeo

First of all. thank you for this pull request. It adds typesafety and reduces wrong use of the system.

A thing which should be discussed is the naming of some objects. I don't like the new package name. I assume it stands for conversion, but conversion would only contain classes for the conversion action, not the conversion results. I suggest the package name wrapper, because the int value is wrapped in a type safe manner and in the end these objects still represent the value, therefore wrap it.

The comments for MountOption.java hold basically for all classes implementing the MaskValueEnum.

Splitting the MaskValueEnum into interface and implementation is overengineering for this library, but acceptable from my point of view.

src/main/java/dev/dokan/dokan_java/constants/dokany/MountError.java

src/main/java/dev/dokan/dokan_java/constants/dokany/MountOption.java

src/main/java/dev/dokan/dokan_java/conv/MaskValueEnum.java

src/main/java/dev/dokan/dokan_java/conv/MaskValueSet.java

JaniruTEC · 2020-05-28T20:56:38Z

A thing which should be discussed is the naming of some objects. I don't like the new package name. I assume it stands for conversion, but conversion would only contain classes for the conversion action, not the conversion results. I suggest the package name wrapper, because the int value is wrapped in a type safe manner and in the end these objects still represent the value, therefore wrap it.

conv does indeed stand for conversion. While I understand your suggestion wrapper wouldn't be a good name either because it's already taken by the #32-API.

The comments for MountOption.java hold basically for all classes implementing the MaskValueEnum.

Same goes for the cases where my comments are missing.

Splitting the MaskValueEnum into interface and implementation is overengineering for this library, but acceptable from my point of view.

For documentation purposes: The idea was to stand in line with the JDK-Collections, where you usually also deal with a lot of interfaces. It also makes swapping out the backend easier in case it's needed.

src/main/java/dev/dokan/dokan_java/conv/MaskValueEnum.java

See: dokan-dev#41 (comment)

infeo · 2020-05-29T22:51:23Z

conv does indeed stand for conversion. While I understand your suggestion wrapper wouldn't be a good name either because it's already taken by the #32-API

Because the other PR is currently only in a draft state, we can act independetly of it from my point of view. But even if we consider it, why is it a problem to add both in the same package? The MaskValueSet and others are also convenience classes for easier use. And finally, nowadays package names can be easily changed due to the great IDE support.

See: dokan-dev#41 (comment)

Renamed getSetFromInt to maskValueSet (and renamed parameter value to mask) See: dokan-dev#41 (comment) Renamed getMaskValue to getMaskingValue See: dokan-dev#41 (comment)

…dokan-java into improved-enumintegers

JaniruTEC · 2020-05-29T23:22:32Z

Because the other PR is currently only in a draft state, we can act independetly of it from my point of view. But even if we consider it, why is it a problem to add both in the same package? The MaskValueSet and others are also convenience classes for easier use. And finally, nowadays package names can be easily changed due to the great IDE support.

Putting too many classes into a package can lead to even more confusion than misleading package names. Also the EnumInteger-API is pretty much decoupled from the other wrapper classes and used over the whole project. That's why I wanted to keep it separate.

JaniruTEC · 2020-05-29T23:24:09Z

Also to have said it:
e042b3d and 3114b10 are the same commit, but 3114b10 has a more detailed message. I somehow messed up with amending the commit. Apologies

Renamed getMaskingValue to maskingValue in MaskValueEnum to match "intValue" in EnumInteger

infeo · 2020-05-30T14:26:50Z

Because the other PR is currently only in a draft state, we can act independetly of it from my point of view. But even if we consider it, why is it a problem to add both in the same package? The MaskValueSet and others are also convenience classes for easier use. And finally, nowadays package names can be easily changed due to the great IDE support.

the EnumInteger-API is pretty much decoupled from the other wrapper classes and used over the whole project. That's why I wanted to keep it separate.

Good point. How about renaming the package to enumInteger, the name the main interface?

JaniruTEC · 2020-05-30T15:08:48Z

@infeo and I agreed on "masking" as package name.

…lueSet Added IntegerConvertible as base interface for EnumInteger and MaskValueSet. IntergerConvertible defines the intValue() method and marks classes that can be converted into ints and vice-versa. Changed EnumInteger to explicitly wrap Enums as special case of IntegerConvertible Changed MaskValueSet to extend IntegerConvertible and renamed method toInt to intValue in accordance to IntegerConvertible Changed method calls to refelct this change

See: dokan-dev#41 (comment)

JaniruTEC added 12 commits May 23, 2020 20:18

Moved EnumInteger to new package conv (conversion)

5809e50

Moved EnumInteger to new package conv (conversion) as first step to refactoring the EnumInteger-System Moved the appropiate Test Updated imports in all accessors

Moved EnumIntegerSet to package conv

028538b

Moved EnumIntegerSet to package conv Moved the appropiate Test Updated imports in all accessors

Added MaskValueEnum as interface for EnumIntegers that can be put int…

19ef584

…o an EnumIntegerSet

Changed appropriate classes to use MaskValueEnum

68d6c4a

Renamed EnumIntegerSet to MaskValueSet

e95d084

Renamed EnumIntegerSet to MaskValueSet Updated depended classes Renamed appropriate Test

Changed MaskValueSet to only accept MaskValueEnums

7270c51

Changed MaskValueSet to only accept MaskValueEnums (and therefore match it's contract)

Changed MaskValueSet to an interface and added class MaskValueSetImpl

5ab0055

Changed MaskValueSet to an interface and added class MaskValueSetImpl as implementor

Added convenience methods to all EnumIntegers

a41caa1

Added #fromInt to EnumIntegers where it was missing

Removed final keyword from MaskValueSetImpl

92159a4

Added additional delegates to MaskValueEnum

e01b376

JaniruTEC marked this pull request as draft May 27, 2020 23:28

JaniruTEC force-pushed the improved-enumintegers branch from 2fcd4f6 to e01b376 Compare May 28, 2020 12:21

Merge remote-tracking branch 'upstream/develop' into improved-enumint…

3ee250e

…egers

JaniruTEC marked this pull request as ready for review May 28, 2020 12:27

infeo requested changes May 28, 2020

View reviewed changes

infeo reviewed May 29, 2020

View reviewed changes

src/main/java/dev/dokan/dokan_java/conv/MaskValueEnum.java Outdated Show resolved Hide resolved

Changed maskValue to maskingValue in all MaskValueEnums

8ef36a9

See: dokan-dev#41 (comment)

JaniruTEC added 3 commits May 30, 2020 01:02

Renamed getSetFromInt to maskValueSet (and changed parameter name)

e042b3d

See: dokan-dev#41 (comment)

Renamed getSetFromInt and getMaskValue

3114b10

Renamed getSetFromInt to maskValueSet (and renamed parameter value to mask) See: dokan-dev#41 (comment) Renamed getMaskValue to getMaskingValue See: dokan-dev#41 (comment)

Merge branch 'improved-enumintegers' of https://github.com/JaniruTEC/…

3585758

…dokan-java into improved-enumintegers

JaniruTEC added 3 commits May 30, 2020 13:28

Renamed getMaskingValue to maskingValue in MaskValueEnum

c6f2df2

Renamed getMaskingValue to maskingValue in MaskValueEnum to match "intValue" in EnumInteger

Fixed MaskValueSetTest and updated it to reformed EnumInteger-API

0272612

Fixed remaining Tests to use reformed EnumInteger-API

3ce089f

JaniruTEC requested a review from infeo May 30, 2020 14:32

Renamed package conv to masking

a3292f1

JaniruTEC added 2 commits May 30, 2020 18:03

Removed delegating methods from EnumInteger

7975114

See: dokan-dev#41 (comment)

JaniruTEC added this to the 2.0 milestone May 31, 2020

JaniruTEC merged commit 43b8d13 into dokan-dev:develop Jun 2, 2020

JaniruTEC deleted the improved-enumintegers branch June 5, 2020 20:11

JaniruTEC removed the request for review from infeo June 16, 2020 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve EnumInteger-System #41

Improve EnumInteger-System #41

JaniruTEC commented May 24, 2020 •

edited

Loading

infeo left a comment

JaniruTEC commented May 28, 2020 •

edited

Loading

infeo commented May 29, 2020

JaniruTEC commented May 29, 2020

JaniruTEC commented May 29, 2020

infeo commented May 30, 2020

JaniruTEC commented May 30, 2020

Improve EnumInteger-System #41

Improve EnumInteger-System #41

Conversation

JaniruTEC commented May 24, 2020 • edited Loading

infeo left a comment

Choose a reason for hiding this comment

JaniruTEC commented May 28, 2020 • edited Loading

infeo commented May 29, 2020

JaniruTEC commented May 29, 2020

JaniruTEC commented May 29, 2020

infeo commented May 30, 2020

JaniruTEC commented May 30, 2020

JaniruTEC commented May 24, 2020 •

edited

Loading

JaniruTEC commented May 28, 2020 •

edited

Loading