New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sortable.js to work with BFA table #3125

Merged
merged 2 commits into from Feb 24, 2018

Conversation

@dobbymoodge
Contributor

dobbymoodge commented Nov 2, 2017

The Failure Cause Management table generated by the Build Failure
Analyzer plugin (https://wiki.jenkins.io/display/JENKINS/Build+Failure+Analyzer)
doesn't render dates in a manner that sortable.js can recognize. This
commit improves the matching and sorting for date-like strings to
accommodate a broader variety of date/time strings, and uses the js Date
class to ensure accurate greater/equal/less than results.

See JENKINS-XXXXX.

Proposed changelog entries

  • Update sortable.js to handle broader range of possible Date/Time strings

Submitter checklist

  • JIRA issue is well described
    No issue exists
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
    This function doesn't add new branches to the existing code path
  • For dependency updates: links to external changelogs and, if possible, full diffs
    No dep updates

Desired reviewers

Improve sortable.js to work with BFA table
The Failure Cause Management table generated by the Build Failure
Analyzer
plugin (<https://wiki.jenkins.io/display/JENKINS/Build+Failure+Analyzer>)
doesn't render dates in a manner that sortable.js can recognize. This
commit improves the matching and sorting for date-like strings to
accommodate a broader variety of date/time strings, and uses the js Date
class to ensure accurate greater/equal/less than results.
* "03-23-99 1:30 PM also ignored content"
* "12-25/1979 13:45:22 always with the ignored content!"
*/
var date_pattern = /^(\d{1,2})[\/-](\d{1,2})[\/-](\d\d|\d\d\d\d)(?:(?:\s*(\d{1,2})?:(\d\d)?(?::(\d\d)?)?)?(?:\s*([aA][mM]|[pP][mM])?))\b/

This comment has been minimized.

@daniel-beck

daniel-beck Nov 3, 2017

Member

Any support for ISO 8601 dates?

This comment has been minimized.

@dobbymoodge

dobbymoodge Nov 3, 2017

Contributor

@daniel-beck that's a good idea but I think it will add significant complexity. My intent was to improve the existing pattern matching but preserve the existing behavior, so I figured ISO 8601 was out of scope.

If I am to add ISO 8601 handling, I'd want to put in unit tests to guarantee I'm not adding any regressions, but I'm not typically a js coder so I'm not sure where to start with that. I'll look through the project today to see if I can find where other JavaScript code is getting tested, or a developer doc, but if you can point me in the right direction it would be a great help. 🙂

I do really like the idea though. Maybe it should go in as a separate pr?

This comment has been minimized.

@dobbymoodge

dobbymoodge Nov 3, 2017

Contributor

Hmm, I wrote that on my phone and didn't proofread it evidently. By "add significant complexity" I meant "add some small complexity which might have significant side-effects". It would be simple to implement but I'd be worried about changing behavior for plugins that depend on sortable.js (and actually have date columns that work with sortable, unlike the Build Failure Analyzer plugin which is what I'm trying to fix in the first place).

This comment has been minimized.

@Wadeck

Wadeck Feb 7, 2018

Contributor

The regex is not really readable (and so will be hard to maintain) but seems to work as expected.

  • 1/4/2017 ignored content
  • 03-23-99 1:30 PM also ignored content
  • 03-23-99 1:30 PM also ignored content
  • 12-25/1979 13:45:22 ignored content
  • 12-25/1979 13:45: ignored content
  • 12-25/1979 13:45 ignored content
  • 12-25/1979 13: ignored content
  • 12-25/1979 1: ignored content
  • 12-25/1979 13:0 ignored content (0 meaning what ? 0 minutes ? in that case we display 00 normally)
  • 12-25/1979 13:4 ignored content (same for any other digit)
  • 12-25/1979 13 ignored content (as the 13 is alone, we do not expect to be an hour)

Support for ISO-8601 will be great of course but concerning your argument of difficulty to test to avoid non-regression, it will be even harder after that PR since the rules seem more lenient.

This comment has been minimized.

@Wadeck

Wadeck Feb 14, 2018

Contributor

Can you suggest an alternative way

After a second look, I cannot find a way to improve readability without splitting into multiple line or dividing the process in multiple steps.

I have found 2 corner cases:

  • 12-25/1979 :45:22 seems to match 45 as hour and 22 as minutes
  • 12-25/1979 ::23 seems to match 23 as hour
    since hour / minute are mark as optional. Seems not the desired behavior but it's only a corner case I think.
@dobbymoodge

This comment has been minimized.

Contributor

dobbymoodge commented Nov 13, 2017

@jenkinsci/code-reviewers review would be appreciated!

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 13, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 14, 2017

I cannot review that. Looks good assuming it works, but it would be great to see explicit unit tests at least. Maybe @kzantow @recena and others could review the JS part

@dobbymoodge

This comment has been minimized.

Contributor

dobbymoodge commented Nov 20, 2017

@kzantow @recena Any ETA on when this can get reviewed?

month = dmatches[1];
day = dmatches[2];
year = parseInt(dmatches[3]);
if (year < 50) {

This comment has been minimized.

@kzantow

kzantow Jan 2, 2018

Contributor

this has always bugged me. is there a different standard for treating 2-digit years? maybe "if it's less than the current 2 digit year, add the first 2 digits of the current year, or add the first 2 digits of the prior century" ?

This comment has been minimized.

@dobbymoodge

dobbymoodge Jan 2, 2018

Contributor

@kzantow I hate it too. I don't know which or how many plugins depend on this script, but I didn't want to break existing behavior.

Obviously, for the most common use case of "sorting lists of things used by jenkins jobs, created by jenkins jobs, or created by users to annotate or classify jenkins jobs and/or the artifacts associated with them", it's hard to imagine any sorted data being older than 2005, which is when at least Wikipedia thinks Hudson was created. I have no idea if this is the only way this script is getting used, or even if it's the dominant use case (although that seems probable).

All that said, if 2 or more devs chime in supporting the notion, I'm happy to drop this in favor of "assume current century for XY <= current_year.XY" or even just "assume current century for 2-digit years". People really shouldn't expect 2-digit years to get any sort of love these days anyhow.

This comment has been minimized.

@Wadeck

Wadeck Feb 7, 2018

Contributor

in terms of non-existing standard:

  • <40 => 20xx, otherwise 19xx for IBM (source)
  • 70 years in the past, 30 in future, considering the year the data was entered for FileMaker (source)
  • .NET let choice to the admin using a control panel (source)
  • 80/20 for Java (source)

It's important to consider especially the kind of data we are using. In our case I would be very surprised that we are storing birthday of users (like a public administration tool) but more focused on present data. In such case (and if we are forced to use 2 digits), the -50/+50 seems appropriate.

@kzantow

kzantow approved these changes Jan 3, 2018

I don't see any issues this will cause, but would very much like to see some sort of test (which may be difficult).

if (itm.match(/^\d\d[\/-]\d\d[\/-]\d\d\d\d$/)) sortfn = this.date;
if (itm.match(/^\d\d[\/-]\d\d[\/-]\d\d$/)) sortfn = this.date;
if (itm.match(date_pattern)) sortfn = this.date;
// if (itm.match(/^\d\d[\/-]\d\d[\/-]\d\d\d\d$/)) sortfn = this.date;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jan 7, 2018

Member

do they need to be removed?

@Wadeck

Wadeck approved these changes Feb 7, 2018

month = dmatches[1];
day = dmatches[2];
year = parseInt(dmatches[3]);
if (year < 50) {

This comment has been minimized.

@Wadeck

Wadeck Feb 7, 2018

Contributor

in terms of non-existing standard:

  • <40 => 20xx, otherwise 19xx for IBM (source)
  • 70 years in the past, 30 in future, considering the year the data was entered for FileMaker (source)
  • .NET let choice to the admin using a control panel (source)
  • 80/20 for Java (source)

It's important to consider especially the kind of data we are using. In our case I would be very surprised that we are storing birthday of users (like a public administration tool) but more focused on present data. In such case (and if we are forced to use 2 digits), the -50/+50 seems appropriate.

* "03-23-99 1:30 PM also ignored content"
* "12-25/1979 13:45:22 always with the ignored content!"
*/
var date_pattern = /^(\d{1,2})[\/-](\d{1,2})[\/-](\d\d|\d\d\d\d)(?:(?:\s*(\d{1,2})?:(\d\d)?(?::(\d\d)?)?)?(?:\s*([aA][mM]|[pP][mM])?))\b/

This comment has been minimized.

@Wadeck

Wadeck Feb 7, 2018

Contributor

The regex is not really readable (and so will be hard to maintain) but seems to work as expected.

  • 1/4/2017 ignored content
  • 03-23-99 1:30 PM also ignored content
  • 03-23-99 1:30 PM also ignored content
  • 12-25/1979 13:45:22 ignored content
  • 12-25/1979 13:45: ignored content
  • 12-25/1979 13:45 ignored content
  • 12-25/1979 13: ignored content
  • 12-25/1979 1: ignored content
  • 12-25/1979 13:0 ignored content (0 meaning what ? 0 minutes ? in that case we display 00 normally)
  • 12-25/1979 13:4 ignored content (same for any other digit)
  • 12-25/1979 13 ignored content (as the 13 is alone, we do not expect to be an hour)

Support for ISO-8601 will be great of course but concerning your argument of difficulty to test to avoid non-regression, it will be even harder after that PR since the rules seem more lenient.

@dobbymoodge

This comment has been minimized.

Contributor

dobbymoodge commented Feb 9, 2018

@Wadeck

The regex is not really readable (and so will be hard to maintain)

I had this thought too. Can you suggest an alternative way to parse such fuzzy data that is comparably concise? Splitting the string along different delimiters and inspecting the output array just orients the complexity vertically instead of horizontally...

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 23, 2018

We had an LTS cut-off, so it's fine to ship it IMHO

@oleg-nenashev oleg-nenashev self-assigned this Feb 23, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 24, 2018

Test failure is unrelated, it appears in other PRs as well

@oleg-nenashev oleg-nenashev merged commit e0070aa into jenkinsci:master Feb 24, 2018

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details

@oleg-nenashev oleg-nenashev referenced this pull request Mar 4, 2018

Open

Implement alphanum sorting in sortable.js #2885

0 of 4 tasks complete

@dobbymoodge dobbymoodge deleted the dobbymoodge:fix_sortable branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment