You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/06/02 23:41:30 UTC
[GitHub] [logging-log4cxx] pm-cfs opened a new pull request #24: Implement mapfilter
pm-cfs opened a new pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24
# Summary
This PR implements the mapfilter for log4cxx.
# Changes
Added implementation of mapfilter
_NOTES:_ the original mapfilter in log4j is a plugin; therefore, it uses a different set of tags entirely. The big question is would we rather copy that implementation syntax even though it would mean much deeper changes to log4cxx, or do we just want the functionality. I went with functionality as this was a pretty trivial addition and uses the existing codebase. My question on the mailing list had one agreeing response and no disagreements.
Using the existing filter syntax, we now have:
` <filter class="MapFilter">
<param name="user.ip" value="127.0.0.1" />
<param name="user.name" value="test2" />
<param name="Operator" value="AND" />
<param name="AcceptOnMatch" value="false" />
</filter>`
# Testing
There is an addition to the filtertests unit test which exercises the mapfilter logic.
You can also add a customized version of the above tag into your logging system and test in-place as we have done.
----------------------------------------------------------------
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] [logging-log4cxx] pm-cfs commented on a change in pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
pm-cfs commented on a change in pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24#discussion_r434634258
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+ using keyvallist = std::map<LogString, LogString>;
Review comment:
done
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+ using keyvallist = std::map<LogString, LogString>;
class LOG4CXX_EXPORT MapFilter: public log4cxx::spi::Filter
{
+ private:
+ bool acceptOnMatch{true};
Review comment:
I think this is complete; please let me know if your compiler has any other issues.
----------------------------------------------------------------
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] [logging-log4cxx] pm-cfs commented on a change in pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
pm-cfs commented on a change in pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24#discussion_r436245461
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+using KeyValList = std::map<LogString, LogString>;
Review comment:
no problem; I switch over to how its done in the existing propertyfilter.h
----------------------------------------------------------------
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] [logging-log4cxx] ams-tschoening merged pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
ams-tschoening merged pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24
----------------------------------------------------------------
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] [logging-log4cxx] rm5248 commented on a change in pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24#discussion_r436224245
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+using KeyValList = std::map<LogString, LogString>;
Review comment:
Using 'using' inside of a header is generally bad practice; in this case though, while it may be okay I don't see the point of creating a new type(this is equivalent to `typedef KeyValList std::map<LogString,LogString>`).
----------------------------------------------------------------
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] [logging-log4cxx] ams-tschoening commented on pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24#issuecomment-637996750
Thanks for the PR, just for reference, this is the thread referred to:
http://mail-archives.apache.org/mod_mbox/logging-dev/202005.mbox/%3CCAAWxiMAKcN1iM0Aj_g0U3-WHMyPVSbZ9_ph9uLGP4o9bvVUfsw%40mail.gmail.com%3E
----------------------------------------------------------------
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] [logging-log4cxx] ams-tschoening commented on a change in pull request #24: Implement mapfilter
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #24:
URL: https://github.com/apache/logging-log4cxx/pull/24#discussion_r434354711
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+ using keyvallist = std::map<LogString, LogString>;
class LOG4CXX_EXPORT MapFilter: public log4cxx::spi::Filter
{
+ private:
+ bool acceptOnMatch{true};
Review comment:
C++11-features like this one need to be changed most likely to support legacy-compilers, simply because I'm still using one myself. :-/
http://docwiki.embarcadero.com/RADStudio/Rio/en/C%2B%2B11_Features_in_the_Classic_Compiler
##########
File path: src/main/include/log4cxx/filter/mapfilter.h
##########
@@ -19,26 +19,87 @@
#include <log4cxx/spi/filter.h>
+#if defined(_MSC_VER)
+ #pragma warning ( push )
+ #pragma warning ( disable: 4251 )
+#endif
+
namespace log4cxx
{
namespace filter
{
+/**
+ * A Filter that operates on a Map.
+ */
+ using keyvallist = std::map<LogString, LogString>;
Review comment:
That type reads somewhat badly, how about using `KeyValList` or `key_val_list` instead? I prefer the former, like most other custom types are named and that scheme is used by your variables as well.
----------------------------------------------------------------
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