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

Implemented derdon's request for NotImplementedError. #801

Merged
merged 8 commits into from
Feb 17, 2014

Conversation

PritishC
Copy link
Member

@PritishC PritishC commented Feb 8, 2014

Used a frozenset to store VSO attributes currently supported.

@@ -213,6 +216,8 @@ def _create(wlk, root, session):
DatabaseEntry.observation_time_end > start))
else:
query = query.filter_by(**{typ: value})
if typ not in SUPPORTED:
raise NotImplementedError("The type so requested has not been implemented yet.")
Copy link
Member

Choose a reason for hiding this comment

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

These lines belong within the else-block! and the check needs to be done before the query might be updated.

Use a better error message such as "The attribute {0!r} is not supported to query a database".format(typ)

@derdon
Copy link
Member

derdon commented Feb 8, 2014

You should not have added test.sqlite. Use git add to put specific files explicitly into the staging area and avoid mistakes such as this one.

@PritishC
Copy link
Member Author

PritishC commented Feb 8, 2014

The test.sqlite file was showing in the untracked portion...I thought I had to add it too. My bad.

@derdon
Copy link
Member

derdon commented Feb 8, 2014

The hidden comment of me is still valid by the way.

@Cadair
Copy link
Member

Cadair commented Feb 8, 2014

@derdon I think we need to modify that test so it dosen't create that file.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 976d9d9 on VaticanCameos:derdonNIError into * on sunpy:master*.

@derdon
Copy link
Member

derdon commented Feb 8, 2014

You meant: "I think I need to modify my test so it doesn't create that file"? :P

@Cadair
Copy link
Member

Cadair commented Feb 8, 2014

ssshh

1. Changed name of frozenset to SUPPORTED_SIMPLE_VSO_ATTRS.
2. Put the raise block in proper position.
3. Changed the error message.
4. Removed test.sqlite.
@PritishC
Copy link
Member Author

PritishC commented Feb 9, 2014

Is 'starred' a supported VSO attribute?

@Cadair
Copy link
Member

Cadair commented Feb 9, 2014

Starred is a supported database attibute but not a VSO one... @derdon ?

@Cadair Cadair added this to the 0.5 milestone Feb 9, 2014
@derdon
Copy link
Member

derdon commented Feb 9, 2014

You're right, @Cadair, Starred has nothing to do with VSO and therefore shouldn't be in the frozenset of supported VSO attributes.

@vaticancameos: Adapt the check so that it says if typ.lower() not in SUPPORTED_SIMPLE_VSO_ATTRS and typ.lower() != 'starred':

1. Removed 'starred' from frozenset.
2. Added check for typ.lower() for 'starred'.
@Cadair
Copy link
Member

Cadair commented Feb 10, 2014

1. Put 'starred' in frozenset SUPPORTED_NONVSO_ATTRS.
2. In the typ.lower() check, used a unionized set from the vso and nonvso attrs.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cb945f5 on VaticanCameos:derdonNIError into * on sunpy:master*.

Cadair added a commit that referenced this pull request Feb 17, 2014
Implemented derdon's request for NotImplementedError.
@Cadair Cadair merged commit 5d90ebb into sunpy:master Feb 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants