<nick> morning all -- I don't have a lot to report this morning so unless there's a desperate need I'll skip standup
<nick> going to be ploughing on with clearing my admin backlog
<wyan> not a lot to report myself either — working on the migration for PR # 2803 and the elasticsearch data migration
<seanh> I'm first going to be going over the feedback on my sidebar tutorial PR, and doing the CSS. After watching Nick's PostgreSQL video
<robertknight> Nothing super pressing from my side. I'm getting on with some refactoring and will pick up the client-side Sentry later today.
<robertknight> seanh: If you get a chance to review the PDF detection changes that would be great.
<seanh> robertknight: Will do, I'll get to that today
<robertknight> In miscellaneous news, I did some investigation yesterday into whether there are tools out there that could help speed up the CoffeeScript -> JS conversion and found https://github.com/eventualbuddha/decaffeinate . Compared to just compiling with CoffeeScript, it has the advantage of being intended to produce editable JS (so it preserves comments, spacing) rather than optimizing for size. It is a CS -> ES2015 converter though, so you have to
ES6isms that you don't want, or adjust the transformation.
<seanh> robertknight: Gonna make a cuppa then look at your PDF detection
<robertknight> seanh: I see what you mean about our ES store
<nick> ARGH CIRCULAR IMPORT
<seanh> robertknight: Added a few comments to the pr, nothing major. I take it you've tested it on a few PDF and HTML documents? (Afaik we don't have a checklist anywhere of cases to test our PDF detection on)
<robertknight> seanh: About docstrings, I'm going to agree with you. Although we're not actively using jsdoc, I do have an example of a tool which I would like to see us use where it will make a difference:
<seanh> Cool, Vim doesn't seem to do that out of the box but there might be a plugin
<seanh> I've come across this problem before as well where you define a function in one part of the file, then elsewhere you do `this.myFunction = myFunction;` On which line do you put the docstring? I think it should go on the function definition, the `this` line could have a normal code comment explaining why the function is being added to this, but it shouldn't have a repeat of the function docstring.
<robertknight> seanh: So the info above comes from TypeScript Language Services, which you can think of as like Jedi for Python. There is IDE/editor integration for Vim. I think it is worth giving modern lighweight IDEs like Atom or Sublime a go though. I still use Vim but for serious editing sessions, the value of a tool which actually understands your code is huge IMO.
<seanh> Hmm, it's still the lack of something equivalent to Syntastic that keeps me away from them. I don't use Jedi or YouCompleteMe or anything in vim, from what I've seen of autocompletion for Python, the ones I've tried in vim at least, it's not as good as trivial word-based completion
<nick> Oh man, I'm stuck in a rabbit warren of horrors and circular imports
<robertknight> nick: Oh? I'm curious ;)
<nick> so I'm trying to indirect all storage access through a single module
<nick> `h.api.storage`
<nick> the problem at the moment is that `create_annotation` and `update_annotation` currently call this `h.api.search.transform.prepare` function
<nick> but on the import path for that function is `h.api.search`
<nick> and the search code needs to expand URIs
<nick> which requires storage access
<nick> so I think now the solution is to move `h.api.search.transform` to `h.api.transform`
<nick> at least temporarily
<nick> because while it used to be search-related it has sadly sprouted a billion other arms and legs now
<seanh> I thought of transform as a place to transform data before it went into or after it came out of our "database"
<nick> well that's undoubtedly what it is now
<seanh> Yeah a lot of what it does looks like it would probably belong in `api` or `api.storage` in the future
<robertknight> But it doesn't have any dependencies on the rest of the search code but the looks of things
<nick> there will be a need for a transformer that maps a normalised model object into a dict for elasticsearch at some point
<nick> which was what that was originally intended to be
<nick> but clearly premature
<nick> anyway
<nick> moving `transform` out of `h.api.search` will work fine, I think
<seanh> `h.api.transform` sounds right to me
<seanh> Ok, off to lunch
lenazun joined the channel
lenazun
hello
hslack
<nick> hello lenazun :)
<nick> (I kicked it this morning.)
<lenazun> :)
<nick> HOW are people still claiming their usernames
<nick> we sent those emails out 4 months ago
<nick> I thought *I* was bad at email...
<lenazun> au contraire. they are very organized and said, “I’ll deal with this next year"
<robertknight> One general thing I have noticed is that test files become difficult to work with when you have more than a couple of levels of nested `describe()`s
<robertknight> Both in the editor (keeping track of indentation) and logically keeping track of which setup code has run before a particular `it()` test, because it is the accumulation of several `beforeEach()` calls spread across the code.
<seanh> Yeah, we should probably avoid nested describes
<seanh> And we should avoid before() and beforeEach() as much as possible in my opinion, too
<robertknight> A couple of levels are OK, but I think no more than that
<robertknight> Well, beforeEach() _is_ useful, but nested beforeEach() is hard to reason about.
<robertknight> I've addressed the other comments in the PR
<robertknight> Mainly, extracting the content detection logic into its own module for testability
<seanh> Well, just be careful that beforeEach()s aren't introducing globals, making test functions harder to understand in isolation, or coupling tests together - these seem to be the most common things that they're used for! There can be good uses for them though
travis-ci joined the channel
travis-ci
hypothesis/h#9570 (alembic-logging - 42acdfd : Nick Stenning): The build passed.
<nick> hmm, @wyan... it looks like replay attacks are still possible using the new reset password code, which I thought was what the `password_updated` field was for?
<nick> that is, in other words: I can use the same reset token multiple times
<wyan> uh...
<wyan> me too, I had checked that, let me check again
<nick> ah, ok, I can look into this
<nick> a first pass makes this look like it's just a synchronisation issue
<nick> my local docker instance has the wrong time
<nick> so postgres has the wrong time
<nick> probably worth using `datetime.datetime.utcnow()` when updating the password field rather than `sa.func.now()`
<nick> obviously in production the database server should have the correct time
lenazun has quit
<robertknight> seanh: I'll give the sidebar a test shortly
<robertknight> Have a 1:1 now
travis-ci joined the channel
travis-ci
hypothesis/h#9576 (PfK5vbcM-add-tutorial-to-sidebar - 0079aa8 : Sean Hammond): The build has errored.
<nick> @seanh: You can probably assume that if I say "x is more $SUBJECTIVE_THING", then that's shorthand for "in my opinion, x is more $SUBJECTIVE_THING" ;)
travis-ci joined the channel
travis-ci
hypothesis/h#9581 (PfK5vbcM-add-tutorial-to-sidebar - 56f3571 : Sean Hammond): The build has errored.
<nick> @seanh: ok, the sidebar tutorial thing looks fine, but we may have to hold off on merging it for now because it involves a data migration
<nick> the alternative would be to reorder the migrations so that we can run the column adds for both this PR and the password reset before we run the constraint migrations
<nick> so each of password reset (A) and sidebar tutorial (B) have three migrations: currently they're ordered A1 A2 A3 B1 B2 B3
<nick> we can merge this, but only if they're ordered A1 B1 A2 B2 A3 B3
travis-ci joined the channel
travis-ci
hypothesis/h#9585 (PfK5vbcM-add-tutorial-to-sidebar - b522e7d : Sean Hammond): The build has errored.