Skip to content

[WIP] Support sorting in *.scss files#88

Closed
tonyganch wants to merge 7 commits intomasterfrom
tg/scss
Closed

[WIP] Support sorting in *.scss files#88
tonyganch wants to merge 7 commits intomasterfrom
tg/scss

Conversation

@tonyganch
Copy link
Copy Markdown
Member

This is a WIP pull request for a nonexistent issue.
It's based on my rework of gonzales.
Please, do not push anything to this branch.

Caveats:

  • I've changed gonzales' API: methods are renamed, all arguments are passed as one object, syntax name is a new optional argument (css is default).
  • Syntax is detected by getting file's extension (link). It's not a good way though, because file nani.css.scss.foo can be a valid scss stylesheet.
  • There are two kinds of comments now: multi-line (/* nani */) and single-line (// nani). More tests should be added to check that SL comments are handled fine.
  • Sorting method (process() in options/sort-order.js) was rewritten almost from scratch. The original idea of extended nodes is nice, so I've used it.
  • One test was commented out (link). Maybe we should move it to some other place.
  • There are now 3 special keywords that can be used in sort order: $variable, $import and $include. I'm a bit tired of some people asking to move includes to the top of a ruleset, and others – to the bottom. So let them decide. Config can look like this:
{ 'sort-order': [
        ['$variable'],
        ['$import', 'color']
] }

Note that $include means both @include and @extend in Sass terms.

  • Tests use afterEach hook to make them more readable and reduce LOC.

tumblr_m50aj7qq0h1r4xjo2o1_500

– Use new method names
– Allow processing of *.scss files
– Check syntax name (css or scss)
- Use one `include` node for both `@include` and `@extend`
– Update tests
Comment thread test/integral.expect.css
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

здесь не должно быть пустой строки

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mishanga , почему?
Мы убираем пустые строки между свойствами, кт принадлежат одной группе?
Даже если они там есть?

@mishanga
Copy link
Copy Markdown
Contributor

В целом, насколько смог вчитаться, всё хорошо.

@mishanga
Copy link
Copy Markdown
Contributor

Добавь, пожалуйста, примеры про SCSS в README.

@gurugray
Copy link
Copy Markdown

и отрбейзиться ;)

@tonyganch
Copy link
Copy Markdown
Member Author

  1. Я считаю, что эта ветка не нуждается в rebase.
  2. Перестаньте писать комментарии на русском, пожалуйста.

@gurugray
Copy link
Copy Markdown

  1. ok
  2. this branch need rebase because of this commit f257b76

@tonyganch
Copy link
Copy Markdown
Member Author

@gurugray, why? It's just a merge commit.

I remember a recent Yandex event.
You had a presentation about git.
I came to you after the presentation and asked about your attitude towards merge commits.
And you answered that they were fine, that it was better to merge branches than rebase them, despite all those "Merge branch master".

I find it interesting now, that one person gives contradictory advice )

@gurugray
Copy link
Copy Markdown

@tonyganch nope, I remember your question — my answer was about long-living feature branches with collaborative work on it with many developers, this is bad case for rebasing, of course

but in your case I don't see any problems to keep main history clean

@tonyganch
Copy link
Copy Markdown
Member Author

@gurugray, oh, I'm sorry if there was any misunderstanding.

However, I don't see any difference between 'Merge branch" and "Merge pull request" commits.
And we keep the latter ones.

@gurugray
Copy link
Copy Markdown

I don't see any difference between 'Merge branch" and "Merge pull request" commits.

there is no difference, but in this pull-request you try to merge branch with merge-commit

however, contributing policy postulated in https://github.com/csscomb/csscomb.js/blob/dev/CONTRIBUTE.md and there is step with rebasing ;)

@tonyganch
Copy link
Copy Markdown
Member Author

Contributing.md is a good point, so I'll remove the commit that annoys you so much.
But I still don't get why.
It would be nice if someone explained why rebasing is a rule.

@gurugray
Copy link
Copy Markdown

ok, I'll write a post about it :)

@tonyganch
Copy link
Copy Markdown
Member Author

@gurugray, no, that's not the way it works.
If you have any concerns about my code, I'm more than happy to hear them.
But be kind to explain your point of view, please.
Here, in this discussion that you've started, not somewhere in your blog.

I may look stubborn, but I will not remove that merge commit blindly, just because you said so.
I want to understand why I should rebase now, not merge, so this situation is not repeated in the future.
So please — once again — explain it to me.
If the decision to make rebase a rule has any good reasons, I would love to know them.
Otherwise I suggest to mark it as an advice.

@gurugray
Copy link
Copy Markdown

ok,

GH give as a model of contributing with PullRequests (PR).

As maintainer I want to keep my development history clear, but PR brings us a full dev-history from branch, so I'm insist to do all possible things to keep main history in readable view.

In your case merge commit do some complexity without any defensible reasons (as said earlier — no reasons to not rebase your branch).

Rebasing feature-branch before accepting PR brings you a readable and, in some case, more controllable history with clear points of branching and merging:
screen001

Of course we shouldn't rebase long-living feature-branches or any public branch which used by other people, so we need to use a merge strategy on they rather then rebase.

And some addition — rebasing PR it's for maintainers, so maintainers should do it without disturbing a PR author.

And if (and maybe) my english is not enough to explain it's clearly — I can (and will) write a russian post about it ;)

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