Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Merged
merged 5 commits into from
Oct 14, 2018
Merged

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26957af
Fix last updated time misleading, only show when file content change
fiennyangeln Oct 8, 2018
5d1e552
Fix prettier
fiennyangeln Oct 8, 2018
100b117
- Simplify regex
fiennyangeln Oct 10, 2018
e3f3ca3
Add test when repositories is moved
fiennyangeln Oct 10, 2018
b38dd21
Use shell.exec mock
fiennyangeln Oct 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions v1/lib/core/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,37 @@ function idx(target, keyPaths) {
);
}

function isNormalInteger(str) {
return /^\+?(0|[1-9]\d*)$/.test(str);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this regex instead: ^\d+$, which is simpler. This regex means the line only contains integers. Check out https://regex101.com/r/mp3HYD/2/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that above (fienny's PR) allow an optional + at the beginning of string

Yangshun's recommended regex doesnt allow that.

If the optional + is intended, use /^\+?\d+$/

Copy link
Contributor

@endiliey endiliey Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if there is normal and abnormal integer from the function naming.

isInt is fine o.o

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numbers are timestamps, why do we need the +? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangshun yup i dont think we need the +

}

function getGitLastUpdated(filepath) {
const timeSpan = spawn
.sync('git', ['log', '-1', '--format=%ct', filepath])
.stdout.toString('utf-8');
// To differentiate between content change and file renaming / moving, use --summary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for only noticing this now, but we should add a try/catch around the code within getGitLastUpdated and return null in the catch case something blows up (for instance, git is not installed on the device, or if the repository is a Mercurial-based one).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually im hoping we remove cross-spawn dependency and use existing shelljs to call the bash command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@endiliey yeah i see some v2 code uses shell.exec()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V1 too. Check v1/lib/publish-pages.js

// To follow the file history until before it is moved (when we create new version), use
// --follow
const result = spawn.sync('git', [
'log',
'--follow',
'--summary',
'--format=%ct',
filepath,
]);

// Format the log results to be ['1234567', 'rename ...', '1234566', 'move ...', '1234565', '1234564']
const records = result.stdout
.toString('utf-8')
.replace(/\n\s*\n/g, '\n')
.split('\n')
.filter(String);

const timeSpan = records.find(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right, but could you test on a file that only has one commit?

Could you also test on files that have 0 commits, aka newly-created files that haven't been checked into Git yet. I suspect we this part might crash.

We don't want the page to blow up for any newly-created pages else they wouldn't be able to preview it.

Copy link
Contributor Author

@fiennyangeln fiennyangeln Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have test that check for file with only one commit, non existing and those with no git timestamp in here v1/lib/core/__tests__/utils.test.js

(item, index, arr) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rewrite this part as such to make the logic clearer:

const timeSpan = records.find((item, index, arr) => {
  const isTimestamp = isNormalInteger(item);
  const isLastItem = index + 2 <= arr.length; // Note: Let's use  <= instead of === to be safer.
  const nextItemIsTimestamp = isNormalInteger(arr[index + 1]);
  return isTimestamp && (isLastItem || nextItemIsTimestamp);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we use <= it will return true for all element except the last

// The correct timeSpan will be a number which is not followed by summary meaning
// the next element is also a number OR it is the last 2 element (since the
// last element will always be the summary -- 'create mode ... ')
isNormalInteger(item) &&
(index + 2 === arr.length || isNormalInteger(arr[index + 1])),
);
if (timeSpan) {
const date = new Date(parseInt(timeSpan, 10) * 1000);
return date.toLocaleString();
Expand Down