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

Aspects: Add more robust setting checks #4582

Merged
merged 7 commits into from Aug 27, 2017
Merged

Conversation

@adhikasp
Copy link
Member

@adhikasp adhikasp commented Jul 29, 2017

Closes #4580 #4463

coalib/settings/Setting.py Outdated
"""
try:
return Language[name]
except AttributeError:

This comment has been minimized.

@jayvdb

jayvdb Aug 16, 2017
Member

This line of your patch is correct, but will need to change if #4643 is merged, as that fixes a fairly fundamental exception problem.

coalib/settings/Setting.py Outdated
@@ -44,6 +45,21 @@ def glob_list(obj, *args, **kwargs):
return obj.__glob_list__(*args, **kwargs)


def language_obj(name):

This comment has been minimized.

@jayvdb

jayvdb Aug 16, 2017
Member

I'm not keen on this name. This name needs to be used in Bear.run() override arg lists, and becomes part of our documentation. Can it simply be language ? And be used as def run(.... , language: language): ; while that looks like it might be wrong, it should work and do the right thing.

The other alternative is that the run argument validator should work correctly with def run(.... , language: Language): , which would require improvements to the argument validator so that dict subclass, like Language, are checked using the value to key-index the dict, and thus KeyError would be the expected exception of incorrect values instead of ValueError.

Also, in order to properly solve #4463 , this PR should include a test which is a Bear using language: language.

Copy link
Member

@jayvdb jayvdb left a comment

Very minor improvements needed

coalib/settings/ConfigurationGathering.py Outdated
return False

if not len(aspects_language):
logging.warning('Language setting is not found in section `{}`. '

This comment has been minimized.

@jayvdb

jayvdb Aug 22, 2017
Member

Language setting .. -> Setting 'language' ..

tests/settings/ConfigurationGatheringTest.py Outdated
logging.getLogger()) as cm:
self.assertTrue(check_aspect_config(
self.section, acquire_settings))
self.assertRegex(cm.output[0], 'Language setting is not found in '

This comment has been minimized.

@jayvdb

jayvdb Aug 22, 2017
Member

start the string on a new line. (also below..)

tests/settings/SettingTest.py Outdated
from coalib.settings.Setting import (
Setting, path, path_list, url, typed_dict, typed_list, typed_ordered_dict,
glob, glob_list)
glob, glob_list, language)

This comment has been minimized.

@jayvdb

jayvdb Aug 22, 2017
Member

on a new line, as it isnt a glob.

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch Aug 23, 2017
@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch Aug 23, 2017
@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch Aug 24, 2017
coalib/settings/ConfigurationGathering.py Outdated
return sections


def check_aspect_config(section, acquire_settings):

This comment has been minimized.

@jayvdb

jayvdb Aug 24, 2017
Member

this isnt a check_.. , as it does UI and it modifies the section

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch 2 times, most recently Aug 27, 2017
@adhikasp
Copy link
Member Author

@adhikasp adhikasp commented Aug 27, 2017

Add new commit to fix #4681

coalib/settings/ConfigurationGathering.py Outdated
return True


def validate_and_inject_language(sections):

This comment has been minimized.

@jayvdb

jayvdb Aug 27, 2017
Member

This can be just _set_section_language . validation is assumed. doesnt need to be part of public API

coalib/settings/Section.py Outdated
@@ -46,24 +46,18 @@ def append_to_sections(sections,

def extract_aspects_from_section(section):
"""
Extracts aspects and their related settings from a section and create an
AspectList from it.
Extracts aspects settings from a section into an AspectList.

This comment has been minimized.

@jayvdb

jayvdb Aug 27, 2017
Member

Singular 'Extract', not plural

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch Aug 27, 2017
@jayvdb
Copy link
Member

@jayvdb jayvdb commented Aug 27, 2017

ack 67ef72b 5f90a8b 25893e2 c8f1405 fc75951 82ec615 9979e16

@jayvdb
jayvdb approved these changes Aug 27, 2017
adhikasp added 7 commits Aug 26, 2017
``aspect`` passed to ``Result`` should be instance of aspectbase.
Move everything related to validating a correct aspect section
into its own function.
Dynamically create `aspects` attribute only when aspects is detected is
prone to many edge case bugs.

Fixes #4681
@adhikasp adhikasp force-pushed the adhikasp:adhikasp/aspect-check branch to a697b09 Aug 27, 2017
@jayvdb
Copy link
Member

@jayvdb jayvdb commented Aug 27, 2017

@rultor merge

@rultor
Copy link
Contributor

@rultor rultor commented Aug 27, 2017

@rultor merge

@jayvdb OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit a697b09 into coala:master Aug 27, 2017
6 of 9 checks passed
6 of 9 checks passed
ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codecov/project 100% (target 100%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit This commit has no issues. :)
Details
review/gitmate/manual This commit was acknowledged.
Details
review/gitmate/pr This PR has no issues. :)
Details
@rultor
Copy link
Contributor

@rultor rultor commented Aug 27, 2017

@rultor merge

@jayvdb Done! FYI, the full log is here (took me 2min)

@adhikasp adhikasp deleted the adhikasp:adhikasp/aspect-check branch Aug 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants