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

Updated application.rst to use latest logging modules and syntax #746

Merged
merged 5 commits into from
Apr 27, 2017

Conversation

RomanMeR
Copy link
Contributor

@codecov
Copy link

codecov bot commented Mar 20, 2017

Codecov Report

Merging #746 into trunk will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            trunk     #746      +/-   ##
==========================================
- Coverage   91.19%   91.18%   -0.02%     
==========================================
  Files         844      844              
  Lines      147851   147851              
  Branches    13065    13065              
==========================================
- Hits       134837   134820      -17     
- Misses      10731    10740       +9     
- Partials     2283     2291       +8

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Thank you very much for the doc update! I was hoping that you could update it to the recommended API for this functionality though, so as to not require users to compose too many layers together at once. (formatEventAsClassicLogText, in particular, is a bit of an implementation detail; it's public API, but it shouldn't be front-and-center).

@@ -104,10 +104,10 @@ Given a file named ``my.py`` with the code:

.. code-block:: python

from twisted.python.log import FileLogObserver
from twisted.logger import FileLogObserver, formatEventAsClassicLogText
Copy link
Member

Choose a reason for hiding this comment

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

The appropriate API to use here is actually testFileLogObserver. Would you mind updating this change?

@RomanMeR
Copy link
Contributor Author

Hi Glyph, thank you for review of my small change. I've made the improvent you proposed locally, but I'm new to git and it seems I made something bad to this pull request, because now it shows some strange things like:

"RomanMeR wants to merge 126 commits into twisted:trunk from RomanMeR:9084-RomanMeR-fix-logging-examples" (which I obviously don't want to)

"RomanMeR added some commits 28 days ago
...
@RomanMeR Now using more modern textFileLogObserver instead of FileLogObserver … … 9c2a1da
@RomanMeR Resolved merge conflict in docs/core/howto/application.rst 43f4f37
"
These last two commits were made today and not 28 days ago...

Git turned out to be very hard to understand and learn for me after all these GUI-based Windows applications like Araxis Merge, Perforce and TFS. It would be very helpful if page TwistedDevelopment had step-by-step git commands not only for the "Submitting a Patch" case, but for "Updating/Re-submitting patch" case also. Maybe you could write such step-by-step guide now? I believe it's easy for experienced Linux developer.

I'm very sorry for wasting your time with mess around this small change.
Please let me know what should I do now with this pull request and/or 9084-RomanMeR-fix-logging-examples branch on GitHub.

@glyph
Copy link
Member

glyph commented Apr 14, 2017

Hi @RomanMeR ! Sorry that you seem to be having a hard time with Git - I'm occasionally sad that it won the DVCS wars; Bazaar had a much nicer interface in many ways. Nevertheless, it's what we're stuck with.

I believe what has happened here is that you used some combination of git rebase and git merge to create duplicate versions of your own commits. I'm not exactly sure how you managed it; the graph is super complex because of the large number of intervening merge commits.

However, fixing it is easier than figuring out exactly what went wrong. Here's what you should do.

First, make sure you have your existing branch checked out locally.

$ git checkout 9084-RomanMeR-fix-logging-examples

Next, we need to point the local branch back at a "good" commit. I used this app to poke around in the history graph to find the appropriate commit, but you could use other tools, like git log --graph. This is the last commit which had a nice linear history and included changes that you clearly wanted to make.

$ git reset --hard 9c2a1dad57e19ebd7667970fa881f4e1ed309f8e

Then, you will need to force-push to update the branch that this PR is pointed at. Your local branch is probably already set up to point at the right location, so this should fix it:

$ git push -f

That should fix it.

If you need to update your branch, you can just click the "update branch" button on this PR.

@RomanMeR RomanMeR force-pushed the 9084-RomanMeR-fix-logging-examples branch from 43f4f37 to 9c2a1da Compare April 17, 2017 07:49
@RomanMeR
Copy link
Contributor Author

Hi @glyph, thank you very much for helping! I performed the steps you described, the only difference is that "git push -f" didn't work for me with the following message:

fatal: The current branch 9084-RomanMeR-fix-logging-examples has no upstream branch.
To push the current branch and set the remote as upstream, use

git push --set-upstream origin 9084-RomanMeR-fix-logging-examples

So I issued this command:

git push -f origin 9084-RomanMeR-fix-logging-examples

It succeeded with the following output:

Total 0 (delta 0), reused 0 (delta 0)
To https://github.com/RomanMeR/twisted.git

  • 43f4f37...9c2a1da 9084-RomanMeR-fix-logging-examples -> 9084-RomanMeR-fix-logging-examples (forced update)

Hope it did what is needed. Please let me know if it worked to upload changes to this pull request correctly.

@glyph
Copy link
Member

glyph commented Apr 18, 2017

Yep, the commit history looks far less confused now. Thank you for updating this!

@glyph
Copy link
Member

glyph commented Apr 18, 2017

Looks like it's broken on Python 3 now though, could you push a fix?

@RomanMeR
Copy link
Contributor Author

I believe failed builds are not due to my changes, but other error addressed by pull request #766: [#9117] Fix twisted.test.test_ftp_options on Python 3. But it's not closed yet

@RomanMeR
Copy link
Contributor Author

Now #766: [#9117] Fix twisted.test.test_ftp_options on Python 3 is merged into trunk and Python 3 build issue is fixed. At the moment things that are not clear to me are:

  1. Why "buildbot/osx10.10-py2.7" check (which is marked as "Required") hasn't reported it's status after several hours
  2. How could the "codecov/project" check report that code coverage is decreased by 1% if my PR doesn't change code but only documentation?

@glyph
Copy link
Member

glyph commented Apr 25, 2017

Why "buildbot/osx10.10-py2.7" check (which is marked as "Required") hasn't reported it's status after several hours
How could the "codecov/project" check report that code coverage is decreased by 1% if my PR doesn't change code but only documentation?

These are both due to the same underlying issue, which is that Twisted's full CI infrastructure (i.e.: our buildbots) is not fully able to defend against malicious parties. Therefore the full suite is only run against branches on the Twisted org's repo, not against every PR. Coverage decreated because e.g. the FreeBSD tests did not run. That OS X build is specifically a mandatory build to point out that we should not be merging things which haven't been through the full suite.

We (project members) have a script (admin/pr_as_branch) which allows us to push a PR to a branch once we have evaluated it not to be malicious; I've done that for this branch now so it should turn green when the builds complete.

@RomanMeR
Copy link
Contributor Author

Yesterday I visited this page and it was telling me that my branch is outdated, so I pressed the "Update branch" button. And only after some time I realized that this action could hide results of full tests suite that you launched manually 2 days ago. I'm very sorry if so.

@glyph
Copy link
Member

glyph commented Apr 27, 2017

I'm very sorry if so.

No worries! Sorry about this convoluted process. I just updated the branch and it's running the new tests.

@glyph
Copy link
Member

glyph commented Apr 27, 2017

OK, codecov is just broken. The coverage number should be rising with buildbot reports, but it isn't. I think I'm going to do an administrator override. Thanks for bearing with us, @RomanMeR .

@glyph glyph merged commit 0aef336 into twisted:trunk Apr 27, 2017
@RomanMeR
Copy link
Contributor Author

Thank you @glyph!

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