You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/05/25 07:56:56 UTC
[GitHub] [commons-csv] dota17 opened a new pull request #73: Refactor csv format
dota17 opened a new pull request #73:
URL: https://github.com/apache/commons-csv/pull/73
refactor CSVFormat withXXX methods using a new constructor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] dota17 commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
dota17 commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-643588024
@garydgregory @melloware
I have completely reworked the code for refactorCSVFormat. This modification is mainly because the new CSVFormat() will be called many times, and there are many parameters, such code doesn't look very goog, and when adding an attribute more code is modified.
It may be better to use the Builder mode, but the Builder mode will cause the API to change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory edited a comment on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all:
I've brought this patch in locally and it does not make sense to me.
The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense.
The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`.
Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat` objects, whether you need them or not, which is not great. With this PR, if you call 100 `CSVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it.
I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1.
I'll provide a proper builder, use it in the current code base, and probably deprecate the old `with` methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633436839
[](https://coveralls.io/builds/31458296)
Coverage increased (+0.1%) to 98.623% when pulling **0c68d6988a2b2c09c9623ccf045bfefe066609a2 on dota17:refactorCSVFormat** into **ab751011e6da0c81ca29b818b125721541db01cc on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633544633
I'm not sure if replacing usage of a type-safe constructor with a type-unsafe `Map` (values are `Object`) is a good thing. What do others think? For future maintainers, it will be easy (IMO) to mess up the cast-typing and the key strings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] melloware commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
melloware commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-644190969
This does look a lot better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-643620659
-1 Reflection is a hack of last resort IMO, not something to use just for style.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory closed pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #73:
URL: https://github.com/apache/commons-csv/pull/73
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633436839
[](https://coveralls.io/builds/31007798)
Coverage increased (+0.1%) to 98.618% when pulling **5efded10c452dc84031caf587cc90527e868b141 on dota17:refactorCSVFormat** into **852e11e4fbaae07d272102f453ea9cfd1684d99d on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-636227475
@melloware
The POJO is the `CSVFormat` itself... so there is nothing to do with this PR IMO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633436839
[](https://coveralls.io/builds/31431170)
Coverage decreased (-0.2%) to 98.327% when pulling **917cabb9f28a3293546ffb98874a06e3b8d6a15a on dota17:refactorCSVFormat** into **9b3046e6d9e0923202160055f4ab197e630ba1c4 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory edited a comment on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all:
I've brought this patch in locally and it does not make sense to me.
The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense.
The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`.
Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat` objects, whether you need them or not, which is not great. With this PR, if you call 100 `CSVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it.
I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1.
I provided a proper builder, used it in the current code base, and deprecated the old `with` methods. The only drawback is that the deprecated methods use a temporary builder which could be changed back to using the contructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory closed pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #73:
URL: https://github.com/apache/commons-csv/pull/73
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory edited a comment on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all:
I've brought this patch in locally and it does not make sense to me.
The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense.
The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`.
Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat` objects, whether you need them or not, which is not great. With this PR, if you call 100 C`SVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it.
I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] melloware commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
melloware commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-643619721
@dota17 i will admit this does look a little nicer but i am not sure introducing reflection just for the sake of rearranging the code.
I do appreciate what you are trying to do though because the code does look "wordy" in its current state.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-874217693
Closing, see git master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] melloware commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
melloware commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-636264407
Agreed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633436839
[](https://coveralls.io/builds/31458705)
Coverage increased (+0.1%) to 98.623% when pulling **9c48fd81e30c9740077bae0c2a773a66f9ee5e11 on dota17:refactorCSVFormat** into **ab751011e6da0c81ca29b818b125721541db01cc on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory edited a comment on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all:
I've brought this patch in locally and it does not make sense to me.
The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense.
The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`.
Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat` objects, whether you need them or not, which is not great. With this PR, if you call 100 `CSVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it.
I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-874217693
Closing, see git master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory edited a comment on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-633544633
I'm not sure if replacing usage of a type-safe constructor with a type-unsafe `Map` (values are `Object`) is a good thing. For future maintainers, it will be easy (IMO) to mess up the cast-typing and the key strings. What do others think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] dota17 commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
dota17 commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-644121337
@garydgregory @melloware
How about this way of modifying code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] melloware commented on pull request #73: Refactor csv format
Posted by GitBox <gi...@apache.org>.
melloware commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-636209730
I agree with @garydgregory it should be an object passed with concrete values not a Hashmap of keys. I think it needs to be a concrete POJO to pass it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #73: Refactor CSVFormat to use an internal builder
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #73:
URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all:
I've brought this patch in locally and it does not make sense to me.
The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense.
The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`.
Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat `objects, whether you need them or not, which is not great. With this PR, if you call 100 C`SVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it.
I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org