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

Remove old version warning #86

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

Conversation

Kagamihime
Copy link

Closes #48

@Kagamihime Kagamihime force-pushed the issue-48 branch 2 times, most recently from bcb9a66 to ae29839 Compare January 31, 2018 22:19
Copy link

@kriti21 kriti21 left a comment

Choose a reason for hiding this comment

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

Use comparing instead of compare in the commit description.

Copy link

@kriti21 kriti21 left a comment

Choose a reason for hiding this comment

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

Also I think the code you have written does not comply to the set style. Did you run coala locally before pushing? Please see the logs for the failing ci tests.

@Kagamihime
Copy link
Author

@kriti21 I've modified compare to comparing ;)
And my code does comply to the set style. The CI tests fail because of coala applying a patch in README.md ([INFO][07:54:11] Applied 'ShowPatchAction' on 'README.md' from 'MarkdownBear'.), I didn't touch it...

@kriti21
Copy link

kriti21 commented Feb 1, 2018

I think the problem starts here. Executing section Markdown... **** MarkdownBear [Section <empty> | Severity NORMAL] **** ! ! The text does not comply to the set style. If the tests were passing before you edited the files, then there is a good chance it is failing because something is going wrong in your code. Did you run the tests before?

@Kagamihime
Copy link
Author

@kriti21 Well, I've found that there's an open issue about this #81

@kriti21
Copy link

kriti21 commented Feb 1, 2018

Oh okay! My bad ;)

Copy link

@ankurg22 ankurg22 left a comment

Choose a reason for hiding this comment

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

Please squash the commits.

@Kagamihime
Copy link
Author

Kagamihime commented Feb 1, 2018

@ankurg22 I have done two changes that are not related to each others, hence the two commits.

@kriti21
Copy link

kriti21 commented Feb 1, 2018

I think they are related to each other. They both solve the same issue after-all. You should squash your commits into one. @Kagamihime

@Kagamihime
Copy link
Author

@kriti21 No, they are not related: the first one is related to the suggestion in the issue We could also add what the current version is in the warning. and the second one is to solve the issue itself.
Otherwise, the commits wouldn't be atomic (see http://api.coala.io/en/latest/Developers/Writing_Good_Commits.html ).
If commits would be related just because they would be solving the same issue, then every PR should be made of one commit.

@Kagamihime Kagamihime changed the title Issue 48 Remove old version warning Feb 9, 2018
This adds the current version of coala in the warning
message when it is outdated.

Closes coala#48
This fixes the old version warning by properly comparing
the version numbers between `result` and `COALA_MIN_VERSION`
instead of comparing these strings lexicographically.

Closes coala#48
@jayvdb
Copy link
Member

jayvdb commented Apr 18, 2018

@gitmate-bot rebase

@jayvdb
Copy link
Member

jayvdb commented Apr 18, 2018

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@jayvdb
Copy link
Member

jayvdb commented Apr 18, 2018

Automated rebase with GitMate.io was successful! 🎉

@Kagamihime
Copy link
Author

CircleCI is still failing?

@jayvdb
Copy link
Member

jayvdb commented Apr 18, 2018

ya, I've forced in the other two pending PRs as they were really basic.
I'd like to get the CI working before merging this one, so that its commit has a pretty green dot beside it in the history.
But that will need to wait for another day (but not too many, the plugins have been neglected too long!).

@Kagamihime
Copy link
Author

I understand. I am not active because I am quite busy for now, but I will remain available if something needs to be done here ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants