Skip to content

Conversation

@filak-sap
Copy link
Contributor

No description provided.

Copy link
Contributor

@jonathanbaker7 jonathanbaker7 left a comment

Choose a reason for hiding this comment

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

Let me know if you have any questions, or need clarification on anything...

README.md Outdated
- [requests == 2.20.0](https://pypi.org/project/requests/2.20.0/)
- [enum34 >= 1.0.4](https://pypi.org/project/enum34/)
- [funcsigs >= 1.0.2](https://pypi.org/project/funcsigs/)
- [lxml >= 3.7.3](https://pypi.org/project/lxml/)
Copy link
Contributor

Choose a reason for hiding this comment

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

These last four items. I believe these libraries would automatically be added by pip. Correct? If so, they aren't necessary here, and you should remove them. This list is just the things the user must have installed to make this work.

Feel free to put these in a separate reference section, near the bottom, if you want to show which libraries you are using for reference purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

But you don't have to.

README.md Outdated
for smith in smith_employees_request.execute():
print(smith.EmployeeID)
```
This projects is issue-free. Should you have any doubts, please, consult [our issue tracker](https://github.com/SAP/python-pyodata/issues).
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. So, all software has bugs. That's a certainty. What you should say here is:

There are no known issues at this time.

README.md Outdated
## How to obtain support

You need to use the method set which accepts key value parameters:
We provide unclaimable support via [GitHub issues](https://github.com/SAP/python-pyodata/issues)
Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely no legal language

This is not a criticism, it's on the advice from our legal team. We aren't lawyers, and lawyers aren't programmers. We don't do legal, they don't do code. So, I try to stay away from anything that sounds like legal lingo.

Please change this to say

We accept bug reports via ..

README.md Outdated

Find more sophisticated examples in the [USAGE](USAGE.md) section.

# Contributing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Contributing
## Contributing

README.md Outdated
they can.

## Tips & tricks
# License
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# License
## License

README.md Outdated

make check
```
the Apache Software License, v. 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the Apache Software License, v. 2
the Apache Software License, v. 2

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to make this a code block. The text is boilerplate, and should read as follows:

Copyright (c) 2019 SAP SE or an SAP affiliate company. All rights reserved.
This file is licensed under the [XXX} except as noted otherwise in the LICENSE file [include link to the LICENSE file in your project repository].”

@filak-sap
Copy link
Contributor Author

@jonathanbaker7 I've added few more commits that should address your findings. Thank you very much!

Copy link
Contributor

@jonathanbaker7 jonathanbaker7 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 good. I've modified one thing - the title - and otherwise I think you are ready to go. It is approved. Once it is merged, please email me so I can make your project public.

@filak-sap filak-sap merged commit c612a09 into SAP:master Feb 12, 2019
jfilak added a commit to jfilak/python-pyodata that referenced this pull request Mar 2, 2019
I didn't expect that metadata will be send in anything else that ASCII.
But we absolutely need to know the charse because of labels.

Northwind returns "Content-Type: application/xml;charset=utf-8"

Closes SAP#1
jfilak added a commit to jfilak/python-pyodata that referenced this pull request Mar 2, 2019
I didn't expect that metadata will be send in anything else that ASCII.
But we absolutely need to know the charse because of labels.

Northwind returns "Content-Type: application/xml;charset=utf-8"

Closes SAP#1
phanak-sap pushed a commit that referenced this pull request Mar 4, 2019
I didn't expect that metadata will be send in anything else that ASCII.
But we absolutely need to know the charse because of labels.

Northwind returns "Content-Type: application/xml;charset=utf-8"

Closes #1
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.

2 participants