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

recursive value expansion in PathExAttMap #74

Open
stolarczyk opened this issue Feb 7, 2021 · 5 comments · Fixed by #75
Open

recursive value expansion in PathExAttMap #74

stolarczyk opened this issue Feb 7, 2021 · 5 comments · Fixed by #75
Assignees
Labels
question Further information is requested

Comments

@stolarczyk
Copy link
Member

Is it intentional that the value expansion is performed only on the top level in attmap.PathExAttMap.__getitem__ method?

This makes the result of the literal value retrieval (lines 3 and 5) different from the same value retrieval (lines 4 and 6) in all the methods that rely on __getitem__, like items, to_dict etc.

The illustration uses a pipestat configuration file with environment variables read by yacman.YacAttMap, which inherits from attmap.PathExAttMap:

In [1]: from yacman import YacAttMap                                                                                                                                                                      

In [2]: y=YacAttMap(filepath="/Users/mstolarczyk/Desktop/testing/pypiper/pipestat_config.yaml")                                                                                                           

In [3]: y.database.port                                                                                                                                                                                   
Out[3]: '5432'

In [4]: y.to_dict(expand=True)["database"]["port"]                                                                                                                                                        
Out[4]: '$PRT'

In [5]: y.port                                                                                                                                                                                            
Out[5]: '5432'

In [6]: y.to_dict(expand=True)["port"]                                                                                                                                                                    
Out[6]: '5432'

In [7]: cat /Users/mstolarczyk/Desktop/testing/pypiper/pipestat_config.yaml                                                                                                                               
name: test
record_identifier: sample1
schema_path: sample_output_schema.yaml
port: $PRT
database:
  name: pipestat-test
  user: postgres
  password: pipestat-password
  host: localhost
  port: $PRT

A possible solution to this would be to change the current _safely_expand function from

def _safely_expand(x):
    return expandpath(x) if isinstance(x, str) else x

to

def _safely_expand(x):
     if isinstance(x, Mapping):
         return {k: _safely_expand(v) for k, v in x.items()}
    return expandpath(x) if isinstance(x, str) else x

I'm wondering if this would cause some serious downstream issues as most of our Python packages import this one.

@stolarczyk stolarczyk added the question Further information is requested label Feb 7, 2021
@vreuter vreuter closed this as completed Feb 7, 2021
@vreuter vreuter reopened this Feb 7, 2021
@vreuter
Copy link
Member

vreuter commented Feb 7, 2021

Sorry, meant to hit "cancel," not "close"

@vreuter
Copy link
Member

vreuter commented Feb 7, 2021

That's a good idea, it's just that it would break the nested attribute access since _safely_expand is used in a few places.

vreuter added a commit to vreuter/attmap that referenced this issue Feb 7, 2021
@vreuter
Copy link
Member

vreuter commented Feb 7, 2021

OK @stolarczyk I opened #75 to try to address this. It looks like it behaves on the test case you provided (though I had to export PRT=5432?) to test. FYI though, I have no idea what #75 may or may not break

@stolarczyk stolarczyk linked a pull request Feb 7, 2021 that will close this issue
@nsheff
Copy link
Contributor

nsheff commented Feb 8, 2021

I believe it may have been intentional that the to_dict methods would not change anything, as they would then provide a way to retrieve the original, non-expanded values.

If we change this behavior, will there be another way to get the non-expanded values?

I don't remember the use case and maybe I'm wrong about the original intention, but I have a vague memory of this.

@stolarczyk
Copy link
Member Author

stolarczyk commented Feb 8, 2021

Yes, there is a way to get the original values. You just omit the 'expand=True' in the method calls.

stolarczyk added a commit that referenced this issue Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants