Skip to content
This repository has been archived by the owner on Oct 1, 2020. It is now read-only.

Removing action_view_rendering and add render_super gem #12

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

Removing action_view_rendering and add render_super gem #12

wants to merge 6 commits into from

Conversation

victorlcampos
Copy link
Contributor

No description provided.

@RaphaelVasconcelos
Copy link

+1

2 similar comments
@thalestpires
Copy link

+1

@annanda
Copy link

annanda commented Nov 27, 2015

+1

@victorlcampos
Copy link
Contributor Author

@dr-itz ??

@dr-itz
Copy link
Owner

dr-itz commented Dec 9, 2015

Busy with $dayjob...

Some things:

  • You should never work on 'master' when contributing to a project. Using a short-lived topic branch is far better, makes commit history look nicer and is a lot easier for you when you have to rebase...
  • Commit "removing action_view_rendering and add render_super gem" just adds the "pry" gem. The commit message says something completely different. Not good.
  • Commit "removing pry gem" actually does what it says. But it only undoes the damage of the previous commit...
  • The conclusion: these two commits must not exist in the PR. Here is where you want to "rebase -i" and drop them both.
  • Commit "removing action_view_rendering and add render_super gem" is OK by itself, but...
  • Looking at the sources of the Gem, there are no tests for the functionality. Sure, the tests in redmine_workflow_enhancement do test it implicitly, but that's (kinda) ok since render_super is part of the plugin. But the Gem needs to test this on its own.

So I need you to add tests to the Gem first and fix up the commits in the PR...

Thanks.

@victorlcampos
Copy link
Contributor Author

Sry for this points. I fixed commits, but I don't know the better way to test this. Could you give me a way?

@victorlcampos
Copy link
Contributor Author

PS: Thx for this tips @dr-itz

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants