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

Sublime dev #38383

Merged
merged 6 commits into from Apr 6, 2018
Merged

Sublime dev #38383

merged 6 commits into from Apr 6, 2018

Conversation

MasseGuillaume
Copy link
Contributor

Resurecting: #26718

@jtojnar
Copy link
Contributor

jtojnar commented Apr 3, 2018

Quoting myself from the original PR:

It looks like it should be possible to share most of the file with stable version. Could it be factored out to common.nix?

@MasseGuillaume
Copy link
Contributor Author

@jtojnar done.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 3, 2018

Great, thanks. Although I still do not like the argument duplication very much. Could you do something like, for example, Firefox does?

@MasseGuillaume
Copy link
Contributor Author

@jtojnar done.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

LGTM, cc maintainers @wmertens, @demin-dmitriy, @zimbatm

x32sha256 = "0dgpx4wij2m77f478p746qadavab172166bghxmj7fb61nvw9v5i";
x64sha256 = "06b554d2cvpxc976rvh89ix3kqc7klnngvk070xrs8wbyb221qcw";
} {};
}
Copy link
Member

Choose a reason for hiding this comment

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

the rec keywords in this file don't seem to be necessary. rec is useful to re-export keys of the current attrset as similarly-named variables into the local scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done: 45e72ed

Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

looks good overall, no objections for a merge

@MasseGuillaume
Copy link
Contributor Author

@zimbatm @jtojnar everything is done on my side.

@zimbatm zimbatm merged commit 15a2dca into NixOS:master Apr 6, 2018
@MasseGuillaume MasseGuillaume deleted the sublime-dev branch April 18, 2018 16:47
@jtojnar jtojnar mentioned this pull request Apr 24, 2018
7 tasks
@knedlsepp
Copy link
Member

@MasseGuillaume The sublime(2) attribute was removed by this PR. I suppose this was by accident.

@jtojnar
Copy link
Contributor

jtojnar commented May 5, 2018

Right, sorry, fixed in 8e18b7c

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

Successfully merging this pull request may close these issues.

None yet

6 participants