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

Clean up some parser tests #2019

Closed

Conversation

Projects
None yet
4 participants
@homelchenko
Copy link
Contributor

commented Jan 25, 2018

Mainly, raising this as a PR to provide a context for the conversation about some ArgumentParser tests

As it seems there used to be a dependency on file system and specific file existence when figuring out what default script file to use.

Another thing, there seem to have been two potential default scripts, 'build.cake' and '.cakefile' (and this is where file existence check might have had sense). Anyways, doesn't seem like '.cakefile' default is around any more

Tests are just cleaned to reflect the current state of the matters

homelchenko added some commits Jan 24, 2018

Remove splitting that never occurs
There seem to be cases with input containing spaces. As it is not the case any more, and split with that parameter set doesn't really help in udnerstanding the test essense, it is now replaces with array initialization expression
Clean and clarify tests regarding default script
As it seems there used to be a dependency on file system and specific file existance when figuring out what default script file to use.

Another thing, there seem to have been two potential default scripts, 'build.cake' and '.cakefile' (and this is where file existance check might have had sense). Anyways, doesn't seem like '.cakefile' default is around any more

Tests are just cleaned to reflect the current state of the matters
@gep13

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

@homelchenko thank you for taking the time to submit this PR. I was just wondering if you had a chance to read through our contributing guidelines. Was there an issue created to go along with this PR? Thanks

@gep13 gep13 changed the title Clean up some parser tests GHXXX: Clean up some parser tests Feb 22, 2018

@homelchenko

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@gep13 I did. I even asked on gitter what approach should be taken for PRs like this one (like not really adding new features but rather of housekeeping nature), and got advice to submit a PR to provide the context along with code for the discussion as it proved to be extremely hard to explain what I meant without pinpointing exact line numbers. So, to answer your next question: no, there's no an open issue.

@devlead devlead requested a review from gep13 Jun 20, 2018

@gep13 gep13 changed the title GHXXX: Clean up some parser tests Clean up some parser tests Mar 21, 2019

@gep13 gep13 added this to the v0.33.0 milestone Mar 21, 2019

@patriksvensson

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Sorry for the delay. Your changes merged in c81be32.
Thank you for your contribution 👍

@homelchenko homelchenko deleted the homelchenko:upstream-fix-parser-tests branch Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.