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

Lets make a pcl #401

Merged
merged 74 commits into from
Mar 19, 2014
Merged

Lets make a pcl #401

merged 74 commits into from
Mar 19, 2014

Conversation

trsneed
Copy link
Contributor

@trsneed trsneed commented Feb 27, 2014

Unfortunately, portable class libraries (or is it library's 🙈) do not support ConcurrentDictionary. So I did some research and added a ConcurrentCache class, inspired by this.

With this, came adding a nuget dependency for the portable project to Microsoft.Net.Http, and Microsoft.Bcl.Immutable.

Added a portable tests project, that should use the portable directive to test the concurrent cache. I stepped through it and everything seemed to work. Not all the integration tests ran, but they were around private repositories, and my test user does not have a premium account.

  • add test portable project
  • add portable project
  • figure out how to implement a concurrent dictionary in a portable library
  • test portable library
  • figure out how to package nuget
  • test nuget package for both portable and net45

Add Octokit-PCL and ConcurrentCache which is used by the PCL in lieu of
the ConcurrentDictionary, which is not available on Windows Phone 8.
Tests for pcl project, looks like this is sticking to the established
convention :)
I forgot about this. My mistake.
Update Readme and add a small description and correctly attribute the
ConcurrentCache.
Tackling this warning:

All projects referencing Octokit-Pcl.csproj must install nuget package
Microsoft.Bcl.Build. For more information, see
http://go.microsoft.com/fwlink/?LinkID=317569. Octokit.Tests-Pcl
@anaisbetts
Copy link
Contributor

And for the tests project I had to add a reference to Microsoft.Bcl.Build, to tackle a warning.

Nope nope nope nope nope nope nope that dumb thing causes infinite annoyance. Veto anything that references Microsoft.Bcl.Build.

@haacked
Copy link
Contributor

haacked commented Feb 27, 2014

paulcbetts: how do we make this portable without it?

@haacked
Copy link
Contributor

haacked commented Feb 27, 2014

Also, this will require updating the Octokit.nuspec file to include the new dependencies. NuGet now supports grouping dependencies by target framework. I don't want the net45 version to have these new dependencies because it doesn't need them.

<dependencies> 
   <group>
      <dependency id="RouteMagic" version="1.1.0" />
   </group>

   <group targetFramework="net40">
      <dependency id="jQuery" />
      <dependency id="WebActivator" />
   </group>

   <group targetFramework="sl30">
   </group>
</dependencies>

@trsneed
Copy link
Contributor Author

trsneed commented Feb 27, 2014

Everything builds and tests fine without it. I just wanted to minimize my warning footprint. 8bbf2bf can be rolled back or not merged. Whichever is easier.

@anaisbetts
Copy link
Contributor

Do any of our public interfaces directly expose HttpClient types? I think we can do this a better way, where the PCL version only declares interfaces, and the platform version actually implements them. That way, we can create a Octokit-WP8 project that references Microsoft.* and leaves them out of all the other builds.

Check out Akavache for an example of this, where the PCL version only sets up interfaces, and the platform version actually brings the implementation. This is the counterintuitive part of PCLs, where the app version will actually replace the PCL version, and the app version can do more.

@haacked
Copy link
Contributor

haacked commented Feb 27, 2014

I think all we expose is ProductHeaderValue which is super easy to abstract.

@KonradIT
Copy link

Hi. Does this mean that we will have a github WP8 app?

@anaisbetts
Copy link
Contributor

Hi. Does this mean that we will have a github WP8 app?

No.

@shiftkey
Copy link
Member

@haacked can we internalize ProductHeaderValue in a separate PR to nudge us towards this Brave New World?

@haacked
Copy link
Contributor

haacked commented Feb 28, 2014

@shiftkey feel free. :)

I'll let the experts figure out Microsoft.Bcl.Build and the ensuing
warnings. 👍

Wrapped ProductHeaderValue class, hope this is right. Made some changes
to ensure the project uses the new, wrapped, ProductHeaderValue.
Working on nuget, this is where I am really weak.
Let's see how this works and handling dependency versioning
Gotta portable nuget package I am finishing testing and figured this
would be consistent. ❇️
@trsneed
Copy link
Contributor Author

trsneed commented Mar 8, 2014

I need a bit of guidance on item 5 I have gone down the rabbit hole on both and think I like implementing a solution that has a separate portable nuget package, instead of trying to manhandle the nuspec. I am a huge fan of the current process for building the nuget package, and I think dirtying up the huspec with hardcoded elements is a bad deal. 🚽

I can do whatever to get this feature 'shipped' just need to know what the consensus is 🐰

After playing in nuspec and the build scripts, I personally prefer building a 'portable' nuget. I have not found a way to build a single package without adding a reference to Octokit itself on Microsoft.Net.Http, though maybe that is what we want?

@haacked
Copy link
Contributor

haacked commented Mar 9, 2014

@trsneed: I'm not sure i follow. What "hardcoded" elements would you need to add to the nuspec here?

I have not found a way to build a single package without adding a reference to Octokit itself on Microsoft.Net.Http,

Well you should only need to add references to Microsoft.* for anything that's not .NET 4.5. In the nuspec, that would be something like:

<dependencies> 
   <group>
      <dependency id="Microsoft.Net.Http" version="1.0.0" />
   </group>

   <group targetFramework="net45">
   </group>
</dependencies>

^^^ What that does is ensure that Microsoft.Net.Http is a dependency when targetting any framework except .NET 4.5. For .NET 4.5, there are no dependencies.

@trsneed
Copy link
Contributor Author

trsneed commented Mar 14, 2014

Thanks, Phil. Appears the nuget server (internal corporate) I was using was a tad out of date. Started using myget to test everything, much better 👍

@trsneed
Copy link
Contributor Author

trsneed commented Mar 14, 2014

Removing WIP tag, this builds and nuget packages deploy as I would expect. Still wrapping my head around git (Paul Betts video helped), hence the abundance of commits.

The only project that references Microsoft packages is the Portable project, as @paulcbetts requested.

@shiftkey
Copy link
Member

This is looking pretty great @trsneed.

I had a stab at this a while ago #284 but I like your approach of doing it side-by-side for the moment.

I'll pull down the changes and have a play, I've already got lots of people asking for this!

@anaisbetts
Copy link
Contributor

@trsneed Great work!

@shiftkey
Copy link
Member

Ok, so the only headache with this PR is that if you try and install Octokit.Reactive into a PCL project you'll get rejected.

I've opened #435 to track this, but it's not a blocker for me.

@trsneed
Copy link
Contributor Author

trsneed commented Mar 19, 2014

Cool! I didn't think of the reactive project until now 😓. I am happy to tackle #435 in a similar manner in the morning (Central Daylight Time), if no one gets to it by then.

A very interesting part of this, to me, was tackling automating nuget and the build steps. That is a very nifty way to build a package without any thought.

shiftkey added a commit that referenced this pull request Mar 19, 2014
@shiftkey shiftkey merged commit 5322709 into octokit:master Mar 19, 2014
@shiftkey
Copy link
Member

Thanks for this.

yes-nick-cage

Now to put together another release.

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.

7 participants