test number_formatter and improve exceptions

include/xlnt/utils/exceptions.hpp

@@ -63,15 +63,6 @@ public:
     invalid_sheet_title(const std::string &title);
 };
 
-/// <summary>
-/// Exception for incorrectly formatted named ranges.
-/// </summary>
-class XLNT_CLASS invalid_named_range : public exception
-{
-public:
-    invalid_named_range();
-};
-
 /// <summary>
 /// Exception when a referenced number format is not in the stylesheet.
 /// </summary>

source/detail/number_formatter.cpp

@@ -160,8 +160,6 @@ bool format_condition::satisfied_by(long double number) const
         return number < value;
     case condition_type::not_equal:
         return number != value;
-    default:
-        return false;
     }
 }
 
@@ -199,24 +197,26 @@ void number_format_parser::parse()
             break;
 
         case number_format_token::token_type::color:
-            if (section.color != format_color::none
+            if (section.has_color
-                || section.condition.type != format_condition::condition_type::none
+                || section.has_condition
-                || section.locale != format_locale::none
+                || section.has_locale
                 || !section.parts.empty())
             {
                 throw std::runtime_error("color should be the first part of a format");
             }
 
+            section.has_color = true;
             section.color = color_from_string(token.string);
             break;
 
         case number_format_token::token_type::locale:
         {
-            if (section.locale != format_locale::none)
+            if (section.has_locale)
             {
                 throw std::runtime_error("multiple locales");
             }
 
+            section.has_locale = true;
             auto parsed_locale = locale_from_string(token.string);
             section.locale = parsed_locale.first;
 
@@ -233,11 +233,12 @@ void number_format_parser::parse()
 
         case number_format_token::token_type::condition:
         {
-            if (section.condition.type != format_condition::condition_type::none)
+            if (section.has_condition)
             {
                 throw std::runtime_error("multiple conditions");
             }
 
+            section.has_condition = true;
             std::string value;
 
             if (token.string.front() == '<')
@@ -439,9 +440,6 @@ void number_format_parser::parse()
             finalize();
 
             return;
-            
-        default:
-            break;
         }
 
         token = parse_next_token();
@@ -524,6 +522,7 @@ void number_format_parser::finalize()
                 fractional_seconds = true;
             }
 
+            //TODO this block needs improvement
             if (part.type == template_part::template_type::month_number
                 || part.type == template_part::template_type::month_number_leading_zero)
             {
@@ -552,8 +551,8 @@ void number_format_parser::finalize()
 
                     if (previous.type == template_part::template_type::text
                         && previous.string == ":"
-                        && (before_previous.type == template_part::template_type::hour ||
+                        && (before_previous.type == template_part::template_type::hour_leading_zero
-                            before_previous.type == template_part::template_type::hour_leading_zero))
+                        || before_previous.type == template_part::template_type::hour))
                     {
                         fix = true;
                         leading_zero = part.type == template_part::template_type::month_number_leading_zero;
@@ -848,9 +847,9 @@ void number_format_parser::validate()
 
     if (codes_.size() > 2)
     {
-        if (codes_[0].condition.type != format_condition::condition_type::none &&
+        if (codes_[0].has_condition
-            codes_[1].condition.type != format_condition::condition_type::none &&
+            && codes_[1].has_condition
-            codes_[2].condition.type != format_condition::condition_type::none)
+            && codes_[2].has_condition)
         {
             throw std::runtime_error("format should have a maximum of two codes with conditions");
         }
@@ -1045,7 +1044,7 @@ number_formatter::number_formatter(const std::string &format_string, xlnt::calen
 
 std::string number_formatter::format_number(long double number)
 {
-    if (format_[0].condition.type != format_condition::condition_type::none)
+    if (format_[0].has_condition)
     {
         if (format_[0].condition.satisfied_by(number))
         {
@@ -1057,7 +1056,7 @@ std::string number_formatter::format_number(long double number)
             return std::string(11, '#');
         }
         
-        if (format_[1].condition.type == format_condition::condition_type::none
+        if (!format_[1].has_condition
             || format_[1].condition.satisfied_by(number))
         {
             return format_number(format_[1], number);
@@ -1125,10 +1124,12 @@ std::string number_formatter::format_text(const std::string &text)
 
 std::string number_formatter::fill_placeholders(const format_placeholders &p, long double number)
 {
+    std::string result;
+
     if (p.type == format_placeholders::placeholders_type::general
         || p.type == format_placeholders::placeholders_type::text)
     {
-        auto result = std::to_string(number);
+        result = std::to_string(number);
 
         while (result.back() == '0')
         {
@@ -1155,13 +1156,11 @@ std::string number_formatter::fill_placeholders(const format_placeholders &p, lo
 
     auto integer_part = static_cast<int>(number);
 
-    switch (p.type)
+    if (p.type == format_placeholders::placeholders_type::integer_only
+        || p.type == format_placeholders::placeholders_type::integer_part
+        || p.type == format_placeholders::placeholders_type::fraction_integer)
     {
-    case format_placeholders::placeholders_type::integer_only:
+        result = std::to_string(integer_part);
-    case format_placeholders::placeholders_type::integer_part:
-    case format_placeholders::placeholders_type::fraction_integer:
-    {
-        auto result = std::to_string(integer_part);
         
         while (result.size() < p.num_zeros)
         {
@@ -1195,16 +1194,13 @@ std::string number_formatter::fill_placeholders(const format_placeholders &p, lo
         {
             result.push_back('%');
         }
-
-        return result;
     }
-
+    else if (p.type == format_placeholders::placeholders_type::fractional_part)
-    case format_placeholders::placeholders_type::fractional_part:
     {
         auto fractional_part = number - integer_part;
-        auto result = fractional_part == 0 ? std::string(".") : std::to_string(fractional_part).substr(1);
+        result = fractional_part == 0 ? std::string(".") : std::to_string(fractional_part).substr(1);
 
-        while (result.back() == '0' || result.size() > (p.num_zeros + p.num_optionals + 1))
+        while (result.back() == '0' || result.size() > (p.num_zeros + p.num_optionals + p.num_spaces + 1))
         {
             result.pop_back();
         }
@@ -1214,7 +1210,7 @@ std::string number_formatter::fill_placeholders(const format_placeholders &p, lo
             result.push_back('0');
         }
 
-        while (result.size() < p.num_zeros + p.num_spaces + 1)
+        while (result.size() < p.num_zeros + p.num_optionals + p.num_spaces + 1)
         {
             result.push_back(' ');
         }
@@ -1223,13 +1219,9 @@ std::string number_formatter::fill_placeholders(const format_placeholders &p, lo
         {
             result.push_back('%');
         }
-
-        return result;
     }
 
-    default:
+    return result;
-        return "";
-    }
 }
 
 std::string number_formatter::fill_scientific_placeholders(const format_placeholders &integer_part,
@@ -1591,9 +1583,6 @@ std::string number_formatter::format_number(const format_code &format, long doub
         case template_part::template_type::day_name:
             result.append(day_names->at(dt.weekday() - 1));
             break;
-
-        case template_part::template_type::bad:
-            throw std::runtime_error("bad format");
         }
     }
     

source/detail/number_formatter.hpp

@@ -36,7 +36,6 @@ namespace detail {
 
 enum class format_color
 {
-    none,
     black,
     blue,
     cyan,
@@ -105,7 +104,6 @@ enum class format_color
 
 enum class format_locale
 {
-    none = 0,
     arabic_saudi_arabia = 0x401,
     bulgarian = 0x402,
     catalan = 0x403,
@@ -218,14 +216,13 @@ struct XLNT_CLASS format_condition
 {
     enum class condition_type
     {
-        none,
         less_than,
         less_or_equal,
         equal,
         not_equal,
         greater_than,
         greater_or_equal
-    } type = condition_type::none;
+    } type = condition_type::not_equal;
 
     long double value = 0;
 
@@ -236,7 +233,6 @@ struct format_placeholders
 {
     enum class placeholders_type
     {
-        bad,
         general,
         text,
         integer_only,
@@ -248,7 +244,7 @@ struct format_placeholders
         scientific_significand,
         scientific_exponent_plus,
         scientific_exponent_minus
-    } type = placeholders_type::bad;
+    } type = placeholders_type::general;
 
     bool use_comma_separator = false;
     bool percentage = false;
@@ -264,7 +260,6 @@ struct number_format_token
 {
     enum class token_type
     {
-        bad,
         color,
         locale,
         condition,
@@ -275,7 +270,7 @@ struct number_format_token
         datetime,
         end_section,
         end
-    } type = token_type::bad;
+    } type = token_type::end;
 
     std::string string;
 };
@@ -284,7 +279,6 @@ struct template_part
 {
     enum class template_type
     {
-        bad,
         text,
         fill,
         space,
@@ -313,7 +307,7 @@ struct template_part
         elapsed_hours,
         elapsed_minutes,
         elapsed_seconds
-    } type = template_type::bad;
+    } type = template_type::general;
 
     std::string string;
     format_placeholders placeholders;
@@ -321,8 +315,11 @@ struct template_part
 
 struct format_code
 {
-    format_color color = format_color::none;
+    bool has_color = false;
-    format_locale locale = format_locale::none;
+    format_color color = format_color::black;
+    bool has_locale = false;
+    format_locale locale = format_locale::english_united_states;
+    bool has_condition = false;
     format_condition condition;
     bool is_datetime = false;
     bool is_timedelta = false;

source/styles/tests/test_number_format.hpp

@@ -1,11 +1,10 @@
 #pragma once
 
 #include <iostream>
+#include <pugixml.hpp>
 #include <cxxtest/TestSuite.h>
 
-#include "pugixml.hpp"
 #include <xlnt/xlnt.hpp>
-#include <detail/number_formatter.hpp>
 
 class test_number_format : public CxxTest::TestSuite
 {
@@ -375,16 +374,6 @@ public:
         TS_ASSERT_EQUALS(formatted, "text");
     }
 
-    void test_bad_part()
-    {
-        xlnt::detail::template_part bad_part;
-        xlnt::detail::format_code bad_code;
-        bad_code.parts.push_back(bad_part);
-        xlnt::detail::number_formatter formatter("", xlnt::calendar::windows_1900);
-        formatter.format_ = { bad_code };
-        TS_ASSERT_THROWS(formatter.format_number(1), std::runtime_error);
-    }
-
     void test_conditional_format()
     {
         xlnt::number_format nf;
@@ -487,7 +476,53 @@ public:
         formatted = nf.format(6, xlnt::calendar::windows_1900);
         TS_ASSERT_EQUALS(formatted, "a---------b");
     }
-    
+
+    void test_placeholders_zero()
+    {
+        xlnt::number_format nf;
+        nf.set_format_string("00");
+        auto formatted = nf.format(6, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "06");
+
+        nf.set_format_string("00");
+        formatted = nf.format(63, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "63");
+    }
+
+    void test_placeholders_space()
+    {
+        xlnt::number_format nf;
+        nf.set_format_string("?0");
+        auto formatted = nf.format(6, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, " 6");
+
+        nf.set_format_string("?0");
+        formatted = nf.format(63, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "63");
+
+        nf.set_format_string("?0");
+        formatted = nf.format(637, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "637");
+
+        nf.set_format_string("0.?");
+        formatted = nf.format(6, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "6. ");
+
+        nf.set_format_string("0.0?");
+        formatted = nf.format(6.3, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "6.3 ");
+        formatted = nf.format(6.34, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "6.34");
+    }
+
+    void test_scientific()
+    {
+        xlnt::number_format nf;
+        nf.set_format_string("0E-0");
+        auto formatted = nf.format(6.1, xlnt::calendar::windows_1900);
+        TS_ASSERT_EQUALS(formatted, "6.1E0");
+    }
+
     void test_locale_currency()
     {
         xlnt::number_format nf;
@@ -627,14 +662,6 @@ public:
         TS_ASSERT_THROWS(nf.format(1.2, xlnt::calendar::windows_1900), std::runtime_error);
     }
 
-    void test_none_condition()
-    {
-        xlnt::detail::format_condition f;
-        f.type = xlnt::detail::format_condition::condition_type::none;
-        f.value = 3;
-        TS_ASSERT(!f.satisfied_by(3));
-    }
-
     void format_and_test(const xlnt::number_format &nf, const std::array<std::string, 4> &expect)
     {
         long double positive = 42503.1234;

source/utils/exceptions.cpp

@@ -52,11 +52,6 @@ invalid_data_type::invalid_data_type()
 {
 }
 
-invalid_named_range::invalid_named_range()
-    : exception("named range not found or not owned by this worksheet")
-{
-}
-
 invalid_file::invalid_file(const std::string &filename)
     : exception(std::string("couldn't open file: (") + filename + ")")
 {