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.

Sign up for GitHub

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

Jump to bottom

coerce tags to lower-case strs #244

Merged
merged 14 commits into from
Apr 28, 2017
Merged

coerce tags to lower-case strs #244

merged 14 commits into from
Apr 28, 2017

Conversation

jgerardsimcock
Copy link
Contributor

@jgerardsimcock jgerardsimcock commented Apr 7, 2017 •
edited
Loading

new normalize_tagsmethod implemented that coerces tags to lower-case strings. This method is called by archive.add_tags and api.create (if tags are added as a metadata param). Documentation has been updated to reflect this change.

>>> arch1 = api.create('my_archive', tags=['TAG1', 'tag2', 42])
>>> arch1.get_tags()
['tag1', 'tag2', '42']
>>>
>>> arch1.add_tags('tAg4', 21)
>>> arch1.get_tags()
['tag1', 'tag2', '42', 'tag4', '21']

@jgerardsimcock jgerardsimcock self-assigned this Apr 7, 2017
@jgerardsimcock jgerardsimcock added this to the 0.7.1 milestone Apr 7, 2017
@coveralls
Copy link

coveralls commented Apr 7, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 96.31% when pulling 3b589c7 on coerce_tags into 7465c47 on master.

delgadom
delgadom requested changes Apr 18, 2017
View reviewed changes
datafs/managers/manager.py Outdated
@@ -485,6 +488,24 @@ def delete_tags(self, archive_name, tags):

self._set_tags(archive_name, updated_tag_list)

def _normalize_tags(self, archive_name, tags):
Copy link
Member

Choose a reason for hiding this comment

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

why is archive_name needed?

examples/ondisk.py Outdated
@@ -118,7 +118,7 @@

>>> with var.get_local_path() as f:
... with xr.open_dataset(f) as ds:
... print(ds) # doctest: +ELLIPSIS
... print(ds) # doctest: +SKIP
Copy link
Member

Choose a reason for hiding this comment

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

why is this being skipped?

Copy link
Contributor Author

@jgerardsimcock jgerardsimcock Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This test would not pass with doctest: +ELLIPSIS
https://travis-ci.org/ClimateImpactLab/DataFS/jobs/219518927

Copy link
Member

Choose a reason for hiding this comment

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

that's the point of tests. Here's the relevant section of the log:

File "/home/travis/build/ClimateImpactLab/DataFS/examples/ondisk.py", line 154, in examples.ondisk
Failed example:
    # Acquire the file from the archive and print the version
    with var.get_local_path() as f:
        with xr.open_dataset(f) as ds:
            print(ds) # doctest: +ELLIPSIS
Expected:
    <xarray.Dataset>
    Dimensions:   (location: 3, time: 731)
    Coordinates:
      * location  (location) |S2 'IA' 'IN' 'IL'
      * time      (time) datetime64[ns] 2000-01-01 2000-01-02 2000-01-03 ...
    Data variables:
        tmax      (time, location) float64 12.98 3.31 6.779 0.4479 6.373 ...
        tmin      (time, location) float64 -8.037 -1.788 -3.932 -9.341 ...
    Attributes:
        version: version 2
Got:
    <xarray.Dataset>
    Dimensions:   (location: 3, time: 731)
    Coordinates:
      * location  (location) |S2 'IA' 'IN' 'IL'
      * time      (time) datetime64[ns] 2000-01-01 2000-01-02 2000-01-03 ...
    Data variables:
        tmax      (time, location) float64 12.98 3.31 6.779 0.4479 6.373 4.843 ...
        tmin      (time, location) float64 -8.037 -1.788 -3.932 -9.341 -6.558 ...
    Attributes:
        version:  version 2
**********************************************************************

It looks like the changes to xarray's print functions slightly altered the print format. The ellipses are just a little too far out. This test should be modified, not skipped!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After modification, tests are still failing. https://travis-ci.org/ClimateImpactLab/DataFS/jobs/223365537

examples/ondisk.py Outdated
@@ -156,7 +156,7 @@
>>> # Acquire the file from the archive and print the version
... with var.get_local_path() as f:
... with xr.open_dataset(f) as ds:
... print(ds) # doctest: +ELLIPSIS
... print(ds) # doctest: +SKIP
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test would not pass with doctest: +ELLIPSIS
https://travis-ci.org/ClimateImpactLab/DataFS/jobs/219518927

docs/api/python/pythonapi.tagging.rst
@@ -11,7 +11,7 @@ View the source for the code samples on this page in :ref:`snippets-pythonapi-ta
Tagging on archive creation
---------------------------

When creating archives, you can specify tags using the ``tags`` argument. You can specify as many as you would like as elements in a list:
When creating archives, you can specify tags using the ``tags`` argument. Tags must be strings and will be coerced to lowercase as a standard. You can specify as many as you would like as elements in a list:
Copy link
Member

Choose a reason for hiding this comment

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

We need to update the CLI version of this document as well.

@coveralls
Copy link

coveralls commented Apr 18, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.05%) to 96.247% when pulling 4452708 on coerce_tags into 7465c47 on master.

delgadom
delgadom reviewed Apr 21, 2017
View reviewed changes
docs/whatsnew.rst
@@ -5,6 +5,8 @@ What's New

These are new features and improvements of note in each release.

.. include:: whatsnew/v0.7.1.txt
Copy link
Member

Choose a reason for hiding this comment

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

I've seen other packages only include the whatsnew of published versions, so this would be included in the TOC once 0.7.1 is released.

delgadom
delgadom approved these changes Apr 21, 2017
View reviewed changes
Copy link
Member

@delgadom delgadom left a comment

Choose a reason for hiding this comment

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

great PR! thanks @jgerardsimcock

delgadom
delgadom approved these changes Apr 28, 2017
View reviewed changes
Copy link
Member

@delgadom delgadom left a comment

Choose a reason for hiding this comment

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

awesome. if this passes tests we should merge & release asap.

@coveralls
Copy link

coveralls commented Apr 28, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 96.379% when pulling b4fa007 on coerce_tags into 488f7e9 on master.

@delgadom delgadom merged commit 22fa902 into master Apr 28, 2017
@delgadom delgadom deleted the coerce_tags branch April 28, 2017 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delgadom delgadom delgadom approved these changes

Assignees

@jgerardsimcock jgerardsimcock

Labels
None yet
Projects
None yet
Milestone
0.7.1
Development

Successfully merging this pull request may close these issues.

tags should be standardized to lower case strings
3 participants
@jgerardsimcock @coveralls @delgadom