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

Update apps by reapplying the templates files #272

Closed
wants to merge 2 commits into from
Closed

Update apps by reapplying the templates files #272

wants to merge 2 commits into from

Conversation

dmathieu
Copy link
Contributor

We need to apply ERB when updating, which cannot reliably be done with a diff content.
Anyone updating can still check it's changes by inspecting the unstaged changed in their repository.

cc @dmcinnes @geemus
Closes #271

@geemus
Copy link
Member

geemus commented May 31, 2016

Probably don't want the .DS_Store files there.

So if I understand/read correctly, this just leaves the erb output-ed files as-is and simply updates templates? Seems reasonable.

@dmathieu
Copy link
Contributor Author

Yes. The changes were previously available in a diff file. They will now be available with git diff.

@geemus
Copy link
Member

geemus commented May 31, 2016

Sounds good

@dmathieu
Copy link
Contributor Author

I've rebased this branch to let the tests pass now that #273 is merged.

require 'uri'

module Pliny::Commands
class Updater
class Updater < Creator
Copy link
Member

Choose a reason for hiding this comment

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

Inheriting from the Creator looks a little strange from me. Was there any specific decision to do this, other than avoid duplicating a few methods? My worry is that there might be unintended side effects by this if something changes in the Creator.

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 I did this to avoid some code repetition. I'm fine removing it though, I just did so.

@gudmundur
Copy link
Member

Thanks @dmathieu. This seems to fix an issue that I just got nudged about. I've added a comment on the change, but otherwise looks good to me.

Damien Mathieu and others added 2 commits May 30, 2017 09:31
We need to apply ERB when updating, which cannot reliably be done with a
diff content.
Anyone updating can still check it's changes by inspecting the unstaged
changed in their repository

Closes #271
Copy link
Member

@gudmundur gudmundur left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes requested earlier. Only a minor one now.

@@ -1,6 +1,7 @@
require 'fileutils'
require 'pathname'
require 'pliny/version'
require 'pliny/commands/creator'
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this line is not needed anymore?

@dmathieu dmathieu closed this May 28, 2018
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