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

Add office365 hook #125

Merged
merged 3 commits into from
Jun 12, 2021
Merged

Add office365 hook #125

merged 3 commits into from
Jun 12, 2021

Conversation

colin-fsa
Copy link
Contributor

No description provided.

@colin-fsa colin-fsa requested review from a team and Legorooj and removed request for a team June 12, 2021 13:56
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

I'd also really like to see a test or two. That way we can send it to CI and test it on all platforms automatically.

CHANGELOG.rst Outdated
@@ -16,6 +16,8 @@ New hooks
<https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/83>`_)
* Added a hook for the 'pygraphviz' library (`#86
<https://github.com/pyinstaller/pyinstaller-hooks-contrib/issues/86>`_)
* Add a hook for ``Office365-REST-Python-Client`` which uses data files in
some methods
Copy link
Member

Choose a reason for hiding this comment

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

This isn't how our changelog works. Editing the changelog directly invariably leads to merge conflicts when multiple PRs add to the same file in the same place. Instead write a news entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for that, I had gotten confused by the instructions in the developer guide. Do you think we could rework that a bit to clarify expectations? I'd be happy to take a stab at revising it, just not sure how to do so.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I've got a half written branch collecting dust somewhere which would do it if I ever finish it. It's also going to explain how to write tests (which I see you've had to guess at - although you guessed right), how to trigger our custom CI/CD, and the copyright header (which most people forget).

@colin-fsa
Copy link
Contributor Author

I'd also really like to see a test or two. That way we can send it to CI and test it on all platforms automatically.

Sure thing! It looks like your test suite works by running code that will throw an exception without the hook so I went with that methodology. I found a way without creating a real request to reproduce the failure, the lines in the test print some stuff to stdout if the hook is present (success) and throw exceptions if the hook is absent (failure).

Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

Nice. Next you've got a linter to appease. I suggest that you run it locally otherwise Github's new approve CI run feature will drive us both nuts.

pip install flake8
git diff -U0 origin/master -- | flake8 --diff -

Depending on how your remotes are set up, you might need to replace origin/master with the last commit ID that you didn't write yourself.

A hook is needed to bring in 3 .xml
data files used by the SharePoint API
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

Tests pass too...

@bwoodsend bwoodsend merged commit 614f985 into pyinstaller:master Jun 12, 2021
@bwoodsend
Copy link
Member

Nice work @colin-fsa

@colin-fsa
Copy link
Contributor Author

Thanks @bwoodsend! Appreciate all of your assistance with learning the ropes.

colin-fsa added a commit to colin-fsa/pyinstaller-hooks-contrib that referenced this pull request Jun 13, 2021
A hook is needed to bring in 3 .xml
data files used by the SharePoint API

Authored-by: cscheriff <62242803+cscheriff@users.noreply.github.com>
@Legorooj Legorooj removed their request for review June 25, 2021 11:42
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