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

Comment text cut #21

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

Conversation

VincentBlondeau
Copy link

It should be better to see the diff now

[a577142] v b and others added 2 commits November 9, 2016 10:09
@girba
Copy link
Member

girba commented Nov 9, 2016

I do not quite understand what this branch shows. If I read the diffs from e433671 I still get lots of changes due to reformatting of code both on the desktop client and online:
e433671

Am I looking at the wrong thing?

@girba
Copy link
Member

girba commented Nov 9, 2016

I see, I think the second commit reverts the effects of the first commit. Is there a quick way for me to look at the concatenated diff?

@VincentBlondeau
Copy link
Author

VincentBlondeau commented Nov 9, 2016

Just click on files changed tab ?
I.e: https://github.com/girba/jdt2famix/pull/21/files

@girba
Copy link
Member

girba commented Nov 10, 2016

He he :). Thanks.

Interesting solution. From what I see the tests did not really change. Could you double check please to ensure that we also get a test for this? This is going to be tricky to maintain in the future and we need good test coverage.

@VincentBlondeau
Copy link
Author

Agreed! Did you try to set up a travis job to ensure the test execution and the coverage on the github?

@girba
Copy link
Member

girba commented Nov 30, 2016

Did you have time to work on the tests already?

@VincentBlondeau
Copy link
Author

I did not had time yet. But I think that configuring properly the travis (with a tag on the readme) + add a test coverage tool like Jacoco could be a first step.

@georgeganea
Copy link
Contributor

The build with travis is now setup. I will merge this once the test is added.

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