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

[WIP] feat: add output directory option #52

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

obihill
Copy link

@obihill obihill commented Apr 23, 2020

Hi,

Thanks for putting this module together. It's been very useful.

I wanted to make a few additions to cover my specific use-cases. I had posted two other requests earlier, but I later realized that I was working off the npm code, which is not up-to-date, so I started again using the current version 1.3.0.

Options functionality has been added to the .write method. Two options are currently available. The output_dir option covers the requirement in Issue #44.

You can see the readme for more information.

Thanks.


Note that a majority of the tests run against pre-generated fixtures, or JSON snippets, that come from Puppeteer's raw output. These are located in the `\test\fixtures` area. To generate one of your own, write or use one of the scripts in the test area `test\sample_js`, and run `bin/puppeteer-js-runner.js` through node, like so:

`node bin/puppeteer-js-runner.js --file=/test/sample_js/sample2.js`.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you accidentally deleted the README file.

Copy link
Author

Choose a reason for hiding this comment

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

@bcoe No, I didn't. I excluded it from the branch because I updated the readme on the fork of your repo as the basis for a temporary npm package. I'm currently using this package to enable the custom output directory feature.

@bcoe
Copy link
Member

bcoe commented Apr 23, 2020

Hey @obihill would be grateful for contributions 🙌, mind potentially splitting this into a few pull requests with descriptions of one distinct thing that is being added or fixed in each one 👌

@obihill
Copy link
Author

obihill commented Apr 27, 2020

Hi @bcoe. 'Not sure how I would simplify it further. If you look at the diff breakdown, the updates seem pretty straightforward.

Alternatively, you could actually just take the code [and rework it], then we could close the pull request.

Basically, all I've done is add optionality to pti.write, then added two options[backup and output_dir] to enable timestamped backups and output to a custom directory respectively.

@bcoe bcoe changed the title Upgrades [WIP] feat: add output directory option May 11, 2020
@bcoe
Copy link
Member

bcoe commented May 11, 2020

@obihill I think if we could keep this PR centered to just the output_dir feature:

  1. describing how you set the option in the README.
  2. adding a couple tests for the feature.

I'd be really happy to land this. If you don't have the time, understandably, I would be happy to rework -- but if you do want to pick up this PR, I'm more than happy to provide feedback.

@mvegter
Copy link
Contributor

mvegter commented May 25, 2020

Hi @bcoe, any chance in having this cherry-picked instead of waiting for the author?

@bcoe
Copy link
Member

bcoe commented Jun 11, 2020

@mvegter yeah, I would be happy to take a patch that uses @obihill's work as a starting point.

@obihill I'm sorry that I didn't see this over the finish line, I appreciate the work you put in (it's a great start).

@obihill
Copy link
Author

obihill commented Jun 11, 2020

@bcoe My apologies for the delayed response to this.

Regarding the options info on the readme, I have something penned on the forked repo here where I've implemented this feature already, also to an npm package.

As I said earlier, I'm ok with you using this as your guide to implement directly i.e. outside this pull request. If you look at the diff, you'll see the changes are not major.

@bcoe
Copy link
Member

bcoe commented Jun 11, 2020

@obihill I just want to make sure you get credit for your contribution 👍

Perhaps you could squash your work on this branch into one commit, and then I can just submit a patch against it?

@obihill
Copy link
Author

obihill commented Jun 11, 2020

@bcoe No, it's fine, no credit sought. Besides, I just made a small tweak.

The substantive commit is this one:
9ee8a49. You can disregard the others. I'll see if I can delete them.

@mvegter
Copy link
Contributor

mvegter commented Jun 11, 2020

Hi @obihill , I cherry-picked your changes as we also had them in our own fork. Perhaps your backup changes can be proposed to and it all will be in a single package.

@obihill
Copy link
Author

obihill commented Jun 12, 2020

Hi @mvegter. Sure, I don't mind. Can you guide me on how to make that happen? Alternatively, the substantive code for the backup feature is line 98 to 112 in lib/output-files.js, with requirement of lib/utils.js for generating the datetime code i.e. if you wanted to cherry-pick.

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