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

Counsel compile env #2030

Closed
wants to merge 6 commits into from
Closed

Counsel compile env #2030

wants to merge 6 commits into from

Conversation

@stsquad
Copy link
Contributor

@stsquad stsquad commented Apr 15, 2019

This contains a bug fix, a little re-factoring and an enhancement to counsel-compile to support different environment variables.

The movement of the (or blddir srcdir) form left us always storing
default directory. Fixes b180abf.
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
This aims to reduce the boilerplate with propertizing sections of the
compile text to which we are about to add more.
@stsquad stsquad force-pushed the counsel-compile-env branch from cfcfad2 to ed236d8 Apr 17, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Apr 17, 2019

@basil-conto thanks for your review, I've pushed a new version of the branch with the changes requested.

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
@stsquad stsquad force-pushed the counsel-compile-env branch from ed236d8 to 2a8f2d7 Apr 17, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Apr 17, 2019

@basil-conto ok v3 pushed.

counsel.el Outdated Show resolved Hide resolved
stsquad added 4 commits Apr 17, 2019
A lot of builds can be influenced by environment
variables. Compilation mode already has support for this by way of
compilation-environment. This adds support for passing that down from
counsel-compile. We will later add a counsel helper for manipulating
the state of counsel-compile-env.
This helper allows us to tweak counsel-compile-env. We don't use
separate actions. We either add the environment variable if it doesn't
already exist or remove it if it does.

There is scope for better validation and dealing with setting ARCH=foo
when we have ARCH=bar in the environment already.
This adds a simple predicate test to filter out bad entries from the
history as well as verify new entries being added to the environment.
We may not have defined a blddir, especially if the user has manually
entered the make invocation. If blddir is nil then just keep
default-directory as is.
@stsquad stsquad force-pushed the counsel-compile-env branch from 2a8f2d7 to 26a6322 Apr 17, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Apr 21, 2019

@basil-conto and @abo-abo are there any outstanding issues I need to address on this PR or is it good to merge?

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 21, 2019

@basil-conto and @abo-abo are there any outstanding issues I need to address on this PR or is it good to merge?

You don't need my approval, and I don't have time or interest for more thorough reviews/designs any more because they're not appreciated around here, but you've addressed all my minor source-level comments, so LGTM.

@abo-abo abo-abo closed this in 3f27e25 May 6, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2019

Thanks all.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2019

@basil-conto

and I don't have time or interest for more thorough reviews/designs any more because they're not appreciated around here

I really appreciate your help with issues and reviews. It's just that I didn't have enough time to fix everything, especially in the last year. But if you notice that something important wasn't given priority, please ping again, I respect your opinion and will try to address it.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 6, 2019

I really appreciate your help with issues and reviews.

Thanks for the vote of confidence, I appreciate it.

It's just that I didn't have enough time to fix everything, especially in the last year.

That is perfectly understandable. I think I speak for all Ivy users when I say I don't expect you to fix everything. (Besides, where's the fun in that? ;) I am grateful for the effort you continue to put into this project, and I don't think I could do a better job in your position.

But if you notice that something important wasn't given priority, please ping again, I respect your opinion and will try to address it.

Thanks. Following your kind response, I feel like I should explain my previous offhand comment a bit. First as a user, and later as someone interested in following its development, I have by now become quite dependent on and invested in Ivy, so I care greatly about doing whatever I can (which isn't much) to help maintain the same high level of quality, consistency, and integration with the rest of Emacs as is expected in Emacs development itself.

My problem is that Ivy follows the "if you care enough about it, just submit a PR" review and development process, and the concerns I express in issues and reviews are either rarely shared by someone willing to do something about it, or even occasionally taken in a negative way by the reviewee. This is normal; it would be very strange and bad indeed if my word were taken as gospel, especially since I'm merely a user and occasional contributor.

But the more lax the review process, the better time reviewing things is spent elsewhere, and the more PRs are needed after the fact. I don't have the time to keep chasing Ivy development after the fact, commenting on or creating PRs for things I think should be done differently (besides, I'm sure it can seem annoying), so my only option at the moment is to step back a bit, and try not to step on anyone's toes.

Thanks again for your kind words and reassurance.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 6, 2019

A larger and less personal but related problem I see with Ivy as a project is that, despite its size and popularity, it has only one maintainer and hasn't retained any collaborators to share the maintenance burden and lend more eyes and thoughts to designs and reviews. I've seen about a handful of interested contributors go through periods of activity, but in the end it's a one-man show, and few seem to stick around. I don't mean to sound negative or critical; rather, I'm hopeful for and eager to see how the project evolves in the future, and how this issue is addressed.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented May 6, 2019

@basil-conto, I feel your pain and share exactly same thoughts 100%. The review process is definitely non-existent here. PRs are just getting merged on a random basis with bugs and it's difficult to track what's going on. Bugs are later on fixed directly on master via little scattered commits. Same goes about new big features. Instead of developing it up to the end where it's tested, reviewed and nearly bug free, and merging it as a single squashed change, the PRs are again being merged randomly and further developments done either on master or other PRs. What's worse some review comments are addressed right before the merge silently without a proper commit on PR, so changes in PR are not the ones that were actually merged. All this really creates mess and discourages contribution.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2019

I think a good way of moving forward is to add more integration tests, maybe generate some documentation from them, annotate them with the design decisions that were taken at the time.

What we have right now is already quite good, there hasn't been a case where the default config was hopelessly broken.

One early design decision that added quite a bit of issues in the long run was the support of multiple regex builders. I still think it was a good decision, since many people do like the two alternative regex builders. But supporting three regex builders instead of one is quite a bit more effort, hence I ask for help, while making sure that at least the default re-builder is supported as best I can.

despite its size and popularity, it has only one maintainer

I'm always happy to accept contributions, and I wouldn't mind adding more maintainers in the future, if it's needed and there are volunteers.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2019

@Alexander-Shukaev

Thanks for your concern. But I would appreciate a less emotional language. We are all here voluntarily and we all try to improve the state of things.

The review process is definitely non-existent here.

This is not the case. I review everything before merging and try to add tests if they are appropriate and I have the time.

PRs are just getting merged on a random basis with bugs and it's difficult to track what's going on.

Having no test coverage for a situation is a pre-existing bug. If a PR passes the existing tests, the code and documentation style, and fits with the design, I'll merge it. Even if it uncovers the mentioned pre-existing bug. It's a good way of moving forward, I think. Especially since we have MELPA-stable, and integration tests for MELPA.

Bugs are later on fixed directly on master via little scattered commits.

Not an issue for me.

Same goes about new big features. Instead of developing it up to the end where it's tested, reviewed and nearly bug free, and merging it as a single squashed change

This is what I try to do most of the time.

the PRs are again being merged randomly and further developments done either on master or other PRs.

Not really a productive comment.

What's worse some review comments are addressed right before the merge silently without a proper commit on PR, so changes in PR are not the ones that were actually merged.
All this really creates mess and discourages contribution.

I don't think that's the case. Yes, sometimes I'll merge a contributor's well-intentioned but semi-broken commit, and amend it with some minor changes that fix it. I don't like the idea of broken commits getting into master. Having a breaking commit and a commit that fixes it right after is not acceptable to me. So I have to either ask the contributor to fix the PR in a way that I already see how it's supposed to be fixed during the review, or apply the fix myself and close the PR. I choose the latter as not to waste my time or the contributor's time. I've been on the other side of the PR, and I would prefer the other maintainers to do the same to my PRs. From my point of view, getting PRs into master faster and with less hassle encourages contribution.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 6, 2019

[BTW, this PR is probably the wrong place to be having this discussion.]

I think a good way of moving forward is to add more integration tests, maybe generate some documentation from them, annotate them with the design decisions that were taken at the time.

Although such additions are always a Good Thing, IMO documentation and tests aren't that lacking, and there are higher priorities elsewhere:

  1. Making various core functionalities more generic and modular. Over the years, the code has been amassing hard-coded quirks like (if (memq caller '(swiper counsel-grep ...)) ...) which make it increasingly hard to maintain and adapt the code, and limit what Ivy can do. The core functions should be as agnostic as possible of what is currently being completed.

  2. Improving the various Counsel search commands as has been discussed before. This includes making their interfaces and customisations consistent, fixing various issues related to their argument handling and usage of the shell and processes, fixing various occur issues, etc.

  3. Triaging issues and PRs as has been discussed before. A lot of currently open issues can be retitled, tagged, merged, closed, etc.

What we have right now is already quite good, there hasn't been a case where the default config was hopelessly broken.

No-one said otherwise, and I doubt Ivy would be so popular if that were the case, but there's always room for improvement.

One early design decision that added quite a bit of issues in the long run was the support of multiple regex builders. I still think it was a good decision, since many people do like the two alternative regex builders. But supporting three regex builders instead of one is quite a bit more effort,

The number of regexp builders shouldn't matter, so long as their machinery is implemented in a sufficiently generic and modular way. This is covered by the aforementioned point (1).

hence I ask for help, while making sure that at least the default re-builder is supported as best I can.

Thanks for your efforts.

I wouldn't mind adding more maintainers in the future

That's good to hear, but only you can make that call.

if it's needed

I think every important project needs (and usually has) at least one other person to whom responsibility can be delegated, and who can help with bug triaging, etc.

and there are volunteers.

You can always count me in, FWIW.

Yes, sometimes I'll merge a contributor's well-intentioned but semi-broken commit, and amend it with some minor changes that fix it.

I have a couple of suggestions for this process. I've noticed you usually amend the last commit in a PR to add a "Fixes #issuenumber" comment to the commit message. This results in a new commit, so Git considers the branch to have "unmerged commits", and Github labels the PR as Closed, rather than Merged. There are two alternative approaches which I think are more beneficial:

  1. Require authors to create the PR against a non-master branch of their fork, so that you can push amendments directly to their branch before merging it upstream.

  2. Instead of adding the "Fixed #issuenumber" comment to the other person's commit, add it to an explicit merge commit.

I prefer (2), as it gives a more informative Git history and keeps everyone happy, but Magit, for example, employs both (1) and (2) as needed.

I don't like the idea of broken commits getting into master. Having a breaking commit and a commit that fixes it right after is not acceptable to me.

I agree.

I choose the latter as not to waste my time or the contributor's time. I've been on the other side of the PR, and I would prefer the other maintainers to do the same to my PRs. From my point of view, getting PRs into master faster and with less hassle encourages contribution.

Fewer hoops means more contributions, that's for sure. But there's a balance to strike between ease of contribution and thoroughness of review. Once something gets merged into master, it becomes increasingly hard with each passing day to change it. Not only because of backward-compatibility, but also because every subsequent change that is added on top of the new feature further entrenches it, making it increasingly hard to change, rethink, redesign, or fix the relevant subsystem. It's like feature creep.

If Magit has taught me anything, it's that contributors more often than not appreciate high standards and thorough reviews, and are more proud of the final version as a result of that. If nothing else, it's a learning experience.

But like I said, there's a balance to strike, and all of this is easier said than done.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 7, 2019

IMO documentation and tests aren't that lacking
Making various core functionalities more generic and modular.

I don't think we can have one without the other. Refactoring code to be more modular will inevitably break things if we're not clear on what the existing code is supposed to do.
But I agree that refactoring is needed and we should carefully proceed with it.

You can always count me in, FWIW.

I've sent you a collaborator invite.

Require authors to create the PR against a non-master branch of their fork, so that you can push amendments directly to their branch before merging it upstream.

I don't fully understand this workflow. Can you give an example?

Instead of adding the "Fixed #issuenumber" comment to the other person's commit, add it to an explicit merge commit.

I've been avoiding merge commits for 5 years now. Let's keep it that way.

This results in a new commit, so Git considers the branch to have "unmerged commits", and Github labels the PR as Closed, rather than Merged.

Is it really a big deal? The PR is closed, the code is in master (properly annotated with a commit message and referencing the PR or the issue), the branch can be deleted.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
We may not have defined a blddir, especially if the user has manually
entered the make invocation. If blddir is nil then just keep
default-directory as is.

Fixes abo-abo#2030
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants