Skip to content
This repository has been archived by the owner on Nov 20, 2019. It is now read-only.

feature/encapsulate-Tests-field #38

Closed
wants to merge 10 commits into from

Conversation

amibiz
Copy link

@amibiz amibiz commented Sep 19, 2016

No description provided.

@tebeka
Copy link
Owner

tebeka commented Sep 20, 2016

Thanks, will take a look soon-ish.

@tebeka
Copy link
Owner

tebeka commented Sep 21, 2016

@amibiz The two main things I'd like to get going are #31 and #32 (the latter don't have to be the yacc tool be we need to separate lexer/scanner and "parser" logic)

@amibiz
Copy link
Author

amibiz commented Sep 23, 2016

@tebeka I left a comment on #31. For #32, I would like to "get there" incrementally, not sure how.

@tebeka
Copy link
Owner

tebeka commented Sep 24, 2016

OK. Sorry but super busy - will try to have a look soon.

}

// AddTest adds a test case to the suite
func (suite *Suite) AddTest(test *Test) {
Copy link
Owner

@tebeka tebeka Oct 13, 2016

Choose a reason for hiding this comment

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

I'm note sure I like this change. I don't see value in hiding the data structure implementation when it's internal. I don't think the extra code is worth it and that it's not idiomatic Go IMO.

If we'd expose some data structures as an API (see #39 ) then we might make Suite and Test an interface.

Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

This change is part of a bigger local refactoring branch. Unfortunately, it has not come to fruition yet, so I thought I will commit and PR early.

@amibiz
Copy link
Author

amibiz commented Oct 15, 2016

As it stands, this PR is not valuable (see @tebeka comment above). Closing it for now.

@amibiz amibiz closed this Oct 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants