<p></p>
<p dir="auto">(gave it a review)</p>
<p dir="auto">Well, I would say that the one change to the public API is that Index->fields can now return hashrefs, so it isn't 100% backward compatible. Someone could be trying to special-case something by grepping the list of fields for a string and that code would stop working.</p>
<p dir="auto">It's not necessarily a bad incompatibility though. The hashrefs only show up if there were details in the index spec that were previously missing, which is current buggy behavior that most people should be happy to have fixed.</p>
<p dir="auto">Two small bugs - I think the comparison of fields in Index.pm <code class="notranslate">around => 'equals'</code> should just do a deep-equals instead of enumerating attributes, and you left a <code class="notranslate">print STDERR</code> in Index.pm <code class="notranslate">fields_with_lengths</code>.</p>
<p dir="auto">Also, I noticed on the Producer/SQLite.pm that it has a comment of stripping off <code class="notranslate">\(\d+\)</code> from field names. Does that mean some parsers include the sizes in the text of the field names? If so, the parsers need updated.</p>
<p dir="auto">Further food-for-thought, what do you think about making the index fields into objects that stringify into the field name? (or stringify into the same string that was passed to the constructor, for more precise backward compatibility?) Then existing code that does string compares would continue to work, and the 'equals' behavior could be implemented on the object. The downside is that it would affect all fields and be more likely to break someone's special cases when inspecting <code class="notranslate">->fields</code></p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/dbsrgits/sql-translator/pull/68#issuecomment-1597418360">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AACJ4AW5EOPKRBSID24DWADXMBYVTANCNFSM4BOZ5HZA">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AACJ4AXA73X6JSKK44IRU73XMBYVTA5CNFSM4BOZ5HZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOL43KW6A.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><dbsrgits/sql-translator/pull/68/c1597418360</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/dbsrgits/sql-translator/pull/68#issuecomment-1597418360",
"url": "https://github.com/dbsrgits/sql-translator/pull/68#issuecomment-1597418360",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>