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

API-425, API-416, API-426: Initial version #1

Merged
merged 1 commit into from
Nov 19, 2015

Conversation

kirkplangrid
Copy link
Contributor

  • This version only supports .Net45. We'll create a new ticket to add other individual platforms.

<?xml version="1.0" encoding="utf-8" ?>
<configuration>
<appSettings>
<add key="PlanGridApiBaseUrl" value="https://plangrid-c-api-dispatcher-test.herokuapp.com" />

Choose a reason for hiding this comment

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

test url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentional. Any reason it should point to prod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this is for unit tests)

@robjailall
Copy link

ooooo

</appSettings>

Ensure that you do not already have an `<appSettings>` node -- if you do, add

Choose a reason for hiding this comment

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

IMO: too much coddling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

Choose a reason for hiding this comment

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

Our customers might appreciate the coddling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robjailall yeah, agreed.

@kirkplangrid
Copy link
Contributor Author

@marchello2000 you may want to take a look too in case we want changes to copyrights, etc.

<authors>kirk</authors>
<owners>kirk</owners>
<projectUrl>https://github.com/plangrid/plangrid-api-net</projectUrl>
<licenseUrl>http://www.opensource.org/licenses/mit-license.php</licenseUrl>

Choose a reason for hiding this comment

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

Do we need to remove the copyright headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll let @marchello2000 chime in on how we want to handle file headers vis a vis an open-source license.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current plan is to go with MIT license like @kirkplangrid has it - @abhikp will double check with legal though.

@josephdwyer
Copy link

👍 - lets wait to hear back on the copyright stuff

@kirkplangrid
Copy link
Contributor Author

@josephdwyer also, we want to make sure versioning in AssemblyInfo is handled automatically.

// by using the '*' as shown below:
// [assembly: AssemblyVersion("1.0.*")]
[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Some way to version this. Either the way we do it in PlanGrid.exe (see \Build\GenerateBuildNumber.ps1) or your method you mentioned yesterday

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I got this working already this morning, but it's part of the jenkins script, so not a part of this pull request.

@kirkplangrid
Copy link
Contributor Author

@marchello2000 wanted to make sure you had a chance to review my last commits before merging.

@marchello2000
Copy link
Contributor

Thanks @kirkplangrid looks good to me! And @abhikp just confirmed, MIT license is GTG!

@kirkplangrid
Copy link
Contributor Author

@marchello2000 last thing, what should the file header be for each file. Is what we have right now OK? Is:

Copyright (c) 2015 PlanGrid, Inc. All rights reserved.

Appropriate for the MIT license? @josephdwyer pointed out that concern, and I'm curious too.

@kirkplangrid
Copy link
Contributor Author

(i.e. what happens if someone does a PR and we merge their code? I Am Not a Lawyer, etc.)

@marchello2000
Copy link
Contributor

Ah, didn't notice/understand that. My impression (from my limited exposure to this in the past life) that this is correct attribution. But... I am not a lawyer :) So... I will check on that. In the mean time it's good since we don't expect anyone to contribute. Btw, the way the repo is setup they can't just push to master, they have to PR the changes, right? I am not sure how this works

@kirkplangrid
Copy link
Contributor Author

@marchello2000 yeah, as a public repo, it's standard Github operating procedures. Which means that we can do what we will, but non-plangrid fold will be forced to fork and submit PRs in the usual open-source fashion.

@marchello2000
Copy link
Contributor

Gotcha, thanks!

@kirkplangrid kirkplangrid merged commit 5916193 into master Nov 19, 2015
@josephdwyer josephdwyer deleted the kirk/api-425-initial-version branch November 24, 2015 23:12
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

4 participants