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.

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

Updated to print out number of archived files #7465 #11175

Merged
merged 9 commits into from
Sep 19, 2019

Conversation

jtpetty
Copy link
Contributor

@jtpetty jtpetty commented Aug 21, 2019

  • Number is capped to a small number to prevent spamming the console
  • Added initial L0 test suite for ArchiveFiles

- Number is capped to a small number to prevent spamming the console
- Added initial L0 test suite for ArchiveFiles

tl.setResourcePath(path.join( __dirname, '..', 'task.json'));
cases.forEach(function(numberOfFiles) {
it('plan for ' + numberOfFiles + ' files', (done: MochaDone) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the description more informative? Hard to deduce from the description what this is testing.

@stephenmichaelf
Copy link
Member

Can we add a screenshot here of what the output looks like?

@stephenmichaelf
Copy link
Member

Need to update minor version in task.json and task.loc.json.

console.log(line);
});

tl.debug('Listing all ' + files.length + ' files to archive:');
for (var i = 0; i < files.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

If we are already iterating and debugging the files here can we combine the two into this one method and just check if we are below the max number of files we want to always print and console.log? Then print the "more files" message if files.length > maxNumberOfLinesToLog?

Then the iterating and printing is all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i probably over complicated it by trying to encapsulate the output into a testable method and did not want to inadvertently mess up the debug output

Choose a reason for hiding this comment

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

I think we can keep it as its own method, just change the plan.push calls to console.log statements

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 am not sure I understand what you are suggesting

Choose a reason for hiding this comment

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

Oh nevermind. I was suggesting pushing the logging into the function and not returning an object, realize now that we lose the testability with that approach. I'm fine with it as is.

@jtpetty
Copy link
Contributor Author

jtpetty commented Aug 27, 2019

archviefiles-screenshot

@@ -18,8 +18,8 @@
"demands": [],
"version": {
"Major": 2,
"Minor": 151,
"Patch": 2
"Minor": 152,
Copy link
Member

Choose a reason for hiding this comment

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

The convention that people use is to make it the number of the sprint the change is made it. I am not sure I entirely agree but that's how it's done :)

Tasks/ArchiveFilesV2/utils.ts Outdated Show resolved Hide resolved
Tasks/ArchiveFilesV2/utils.ts Outdated Show resolved Hide resolved
console.log(line);
});

tl.debug('Listing all ' + files.length + ' files to archive:');
for (var i = 0; i < files.length; i++) {

Choose a reason for hiding this comment

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

I think we can keep it as its own method, just change the plan.push calls to console.log statements

@jtpetty jtpetty merged commit 9b4ea36 into master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants