Skip to content

Convert needlessly public methods and properties to internal. #3

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

Closed
wants to merge 4 commits into from

Conversation

dbart01
Copy link
Contributor

@dbart01 dbart01 commented May 3, 2017

The GraphQL support file is generally included in the module that also contains the generated files. There's no need to declare many internal methods and properties as public. Doing so increases the risk of end-users incorrectly using the API and pollutes the auto-complete namespace.

@dbart01 dbart01 requested a review from dylanahsmith May 3, 2017 15:36
@dylanahsmith
Copy link
Contributor

The tests fail since it is testing the support package as a separate package.

Doing so increases the risk of end-users incorrectly using the API and pollutes the auto-complete namespace.

Is this something that has come up, or just an abstract problem?

If we want to distribute it as a swift package in the future, then we will need to revert these changes.

@dbart01 dbart01 force-pushed the buy-sdk branch 2 times, most recently from 2f0141e to 91da590 Compare May 18, 2017 16:07
// Generated from <%= script_name %>
//
// <%= type.name %>.swift
// Buy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these instances of "Buy" sitting around?

// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the inline license necessary? Why not just license the whole repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a license for the generated code, which will end up being in another repo. I'm not sure if the license in the generated code is necessary though.

- add InputValue type and extensions
- modify generation to use InputValue enums in serialize()
- declare all optional values in Input objects at InputValue<T>
@BenEmdon
Copy link
Contributor

BenEmdon commented Sep 7, 2017

Is the purpose of this PR to allow sending explicit nulls? This is what #8 and #12 are trying to achieve

@dbart01
Copy link
Contributor Author

dbart01 commented Sep 7, 2017

The purpose was originally to sync changes that we needed to make on Buy SDK. This was back in March. As for the recent commit with explicit null support, @sav007 and I had a long discussion about the kind of API we want to expose in Mobile Buy SDK and this is the outcome. You're more than welcome to adopt it as well.

@dbart01 dbart01 closed this Sep 7, 2017
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.

3 participants