[GitHub] [ant] bodewig commented on pull request #141: #64855 Add filterbeforeconcat option to Concat
bodewig commented on pull request #141: URL: https://github.com/apache/ant/pull/141#issuecomment-739484127 I like the approach of moving the logic for fixing the lat lines to a separate filter. Kudos for adding tests ;-) A different approach could be to allow an alternative set of filters to be applied before concatenating, i.e. allowing users to define two sets of filters one to apply on each input stream individually and one to be applied to the merged stream. Naming could become an issue (and I've got a long track of picking bad names myself). I'm not asking you to change your PR, I'd rather like to discuss the different approaches to see which would be best. So what do you 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] TheConstructor commented on pull request #141: #64855 Add filterbeforeconcat option to Concat
TheConstructor commented on pull request #141: URL: https://github.com/apache/ant/pull/141#issuecomment-739532616 I briefly thought about that, but thought it would at another layer of complexity. In the end you could wrap a into another without `filterbeforeconcat`, if you want to filter before and after concatenation. 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] TheConstructor edited a comment on pull request #141: #64855 Add filterbeforeconcat option to Concat
TheConstructor edited a comment on pull request #141: URL: https://github.com/apache/ant/pull/141#issuecomment-739532616 I briefly thought about that, but thought it would add another layer of complexity. In the end you could wrap a `` into another `` without `filterbeforeconcat`, if you want to filter before and after concatenation. 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] bodewig commented on pull request #140: #64854 Add preserveduplicates option to ResourceList
bodewig commented on pull request #140: URL: https://github.com/apache/ant/pull/140#issuecomment-739559178 You removed the `volatile` attribute `cached´ from `ResourceList`. Th way code is structured now there is a race-condition where one thread my be using a cache that hasn't been initialized completely. I'm afraid you need to bring back `cached` redundant as it may seem. Apart from that I've only got a few nits: * I'm not sure about the name `WriteableResourceCollection` - `AppendableResourceCollection` maybe? * please add since markers with 1.10.10 to new classes/methods and to the manual * more importantly, add yourself to CONTRIBUTORS (plain text) and contributors.xml. The files are supposed to be ordered lexicographically by first name. Many thanks. 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] bodewig commented on pull request #141: #64855 Add filterbeforeconcat option to Concat
bodewig commented on pull request #141: URL: https://github.com/apache/ant/pull/141#issuecomment-739559350 Yes, you are right. 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] TheConstructor commented on pull request #140: #64854 Add preserveduplicates option to ResourceList
TheConstructor commented on pull request #140: URL: https://github.com/apache/ant/pull/140#issuecomment-739563468 I checked that `cachedResources` is only ever accessed through `cache()`, which is `synchronized`. Is this really a problem? Would declaring `cachedResources` `volatile` and/or assigning the field just after populating the cache help in your opinion? To your nits: - I am happy to change the name - I wasn't quite sure which version could be next, so I thought it would be good to have something in place, where I can pick up work. -> will insert the version - Ok 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] bodewig commented on pull request #140: #64854 Add preserveduplicates option to ResourceList
bodewig commented on pull request #140: URL: https://github.com/apache/ant/pull/140#issuecomment-739723211 I completely overlooked `cache` was synchronized. Since all read an write operations on `cachedResources` are synchronized, I don't think a problem exists there. Sorry. 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] bodewig merged pull request #140: #64854 Add preserveduplicates option to ResourceList
bodewig merged pull request #140: URL: https://github.com/apache/ant/pull/140 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] bodewig merged pull request #141: #64855 Add filterbeforeconcat option to Concat
bodewig merged pull request #141: URL: https://github.com/apache/ant/pull/141 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] [ant] TheConstructor commented on pull request #140: #64854 Add preserveduplicates option to ResourceList
TheConstructor commented on pull request #140: URL: https://github.com/apache/ant/pull/140#issuecomment-739739737 @bodewig Thank you again for looking into this and my other PR! 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
