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.

Sign up for GitHub

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

Jump to bottom

fix: drop actionpack from runtime requirements #34

Merged
merged 1 commit into from
Oct 22, 2019
Merged

fix: drop actionpack from runtime requirements #34

merged 1 commit into from
Oct 22, 2019

Conversation

prikha
Copy link
Contributor

@prikha prikha commented Oct 11, 2019

At this moment distributed tracing has an active_support runtime dependency. Which might not be welcome in some Ruby projects.

It would be nice to leave it at least a development dependency or drop completely.

@prikha
Copy link
Contributor Author

prikha commented Oct 14, 2019

@noaelad would you mind taking a look at currently open PR's. There are a few including this that remove friction for those playing around with the library.

noaelad
noaelad previously requested changes Oct 14, 2019
View reviewed changes
Copy link

@noaelad noaelad left a comment

Choose a reason for hiding this comment

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

Thanks for flagging @prikha. This PR looks good to me, thanks for reducing the footprint. Would you mind removing it as a dev dependency as well? I don't think it's used anywhere else.
I'll take a look at the other open PRs later this week, apologies for the slowness on these.

@prikha
Copy link
Contributor Author

prikha commented Oct 15, 2019

There is spec for ActionController::Param coercion if I am not mistaken.

@prikha prikha force-pushed the let-actionpack-be-development-dependency-indeed branch from 64de56e to 1ebf3ff Compare October 21, 2019 11:56
@prikha prikha changed the title Drop actionpack from runtime requirements chore: drop actionpack from runtime requirements Oct 21, 2019
@prikha
Copy link
Contributor Author

prikha commented Oct 21, 2019

Noa @noaelad can you give me a hint how to trigger the builds though. I feel like the spec is legit. Are you sure we want to drop it?

@noaelad
Copy link

noaelad commented Oct 21, 2019

@prikha I think you just need to login to Circle CI with your github account and maybe set it up for your fork of this repo. This PR is an example of a fork running the test suite:
#16
Let me know if you run into trouble getting that working

@noaelad
Copy link

noaelad commented Oct 21, 2019

You are right about the spec, sorry I missed that one. Let's keep it as a dev dependency then.

@prikha
Copy link
Contributor Author

prikha commented Oct 22, 2019

@noaelad what do I need to do to:
a) make CI run
b) have an approval and merge the PR
?

@prikha prikha closed this Oct 22, 2019
@prikha prikha reopened this Oct 22, 2019
rylanc
rylanc approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@rylanc rylanc left a comment

Choose a reason for hiding this comment

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

This looks great! One thing: this looks to me like a bugfix, since active_support isn't listed as a dependency in the gemspec. If you tried to use tracing in a non-Rails env, it wouldn't work. I'm going to rename this as a fix

@rylanc rylanc changed the title chore: drop actionpack from runtime requirements fix: drop actionpack from runtime requirements Oct 22, 2019
@rylanc rylanc merged commit 64acd27 into Gusto:master Oct 22, 2019
rylanc pushed a commit that referenced this pull request Oct 22, 2019
@semantic-release-bot
chore(release): 0.5.1 [skip ci]
891637e 
## [0.5.1](v0.5.0...v0.5.1) (2019-10-22)

### Bug Fixes

* drop actionpack from runtime dependencies ([#34](#34)) ([64acd27](64acd27))
@rylanc
Copy link
Contributor

rylanc commented Oct 22, 2019

🎉 This PR is included in version 0.5.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

@rylanc rylanc rylanc approved these changes

@noaelad noaelad noaelad left review comments

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prikha @noaelad @rylanc