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

wrap data source use/unuse in transaction so we are sure of the unuse #491

Merged

Conversation

@mpapis
Copy link
Member

@mpapis mpapis commented Nov 3, 2014

extracted from #481

@bobthecow
Copy link
Member

@bobthecow bobthecow commented Nov 3, 2014

👍

Loading

@@ -299,6 +299,14 @@ def self.config_filename_for_cwd

private

# make sure datasources are unloaded properly even on trouble
Copy link
Member

@ddfreyne ddfreyne Nov 3, 2014

Choose a reason for hiding this comment

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

I’d reword this. How about:

Executes the given block, making sure that the datasources are available for the duration of the block.

Mentioning the “trouble” bit is unnecessary since it is clear enough that this method uses the idiomatic Ruby resource cleanup style.

Loading

Copy link
Member

@ddfreyne ddfreyne Nov 3, 2014

Choose a reason for hiding this comment

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

And yes, I do care about documentation :)

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 3, 2014

Apart from comment, sweet 👍

Loading

@mpapis mpapis force-pushed the features/ensure_datasources_unuse branch from d52b6fe to fc7d1e5 Nov 3, 2014
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 3, 2014

I’d leave out the “and are always unused” — it’s pretty confusing.

Loading

@mpapis mpapis force-pushed the features/ensure_datasources_unuse branch from fc7d1e5 to b281822 Nov 3, 2014
@mpapis
Copy link
Member Author

@mpapis mpapis commented Nov 3, 2014

updated

Loading

ddfreyne added a commit that referenced this issue Nov 3, 2014
Wrap data source #use/#unuse in transaction
@ddfreyne ddfreyne merged commit 684604f into nanoc:release-3.7.x Nov 3, 2014
1 check was pending
Loading
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 3, 2014

Thanks!

Loading

@mpapis
Copy link
Member Author

@mpapis mpapis commented Nov 3, 2014

my pleasure :)

Loading

@bobthecow
Copy link
Member

@bobthecow bobthecow commented Nov 3, 2014

I’d leave out the “and are always unused” — it’s pretty confusing.

Maybe "and are released after completion" or something?

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 3, 2014

Maybe "and are released after completion" or something?

They’re not necessarily released, since this uses reference counting.

Loading

@bobthecow
Copy link
Member

@bobthecow bobthecow commented Nov 3, 2014

gotcha. okay. leave it out then :)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants