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

edit-list: handle config consistently #448

Closed
wants to merge 2 commits into from
Closed

Conversation

dimidd
Copy link
Contributor

@dimidd dimidd commented May 9, 2018

Read the config file regardless of what dir the script was called.

Signed-off-by: Dimid Duchovny dimidd@gmail.com

Read the config file regardless of what dir the script was called.

Signed-off-by: Dimid Duchovny <dimidd@gmail.com>
@Humbedooh
Copy link
Member

I'm generally in favor of this change, but it may be breaking existing behavior, so it should at the very least be noted in the changelog that this is a potential breaking change.

@dimidd
Copy link
Contributor Author

dimidd commented May 9, 2018

Perhaps to prevent breaking existing code we can check if the PWD has ponymail.cfg and perefer it over the one in the tools dir.

@sebbASF
Copy link
Contributor

sebbASF commented May 9, 2018

Agreed: only check tools/ if cannot find config in PWD.
I don't see how that can break anything except a test that expects to fail because it cannot find the file.

[Later] As per below, I now think it's confusing to pick up config from the current directory

@Humbedooh
Copy link
Member

I think that's a very good compromise :)

@sebbASF
Copy link
Contributor

sebbASF commented May 14, 2018

I've had a look at how the other tools behave.

Currently, archiver.py and import-mbox.py both read ponymail.cfg from the parent directory.
Whereas the other tools/ scripts read from the CWD.

I think the current situation is confusing: I would expect all the tools to behave the same way.

So although this will change the behaviour, I now think it might be best for all the tools to work the same as archiver and import-mbox.

@dimidd
Copy link
Contributor Author

dimidd commented May 14, 2018 via email

@sebbASF
Copy link
Contributor

sebbASF commented May 14, 2018

elastic.py was an attempt to extract some of the common processing into a separate module.
However perhaps it should be split further.

Allow handling pony config files consistently.

Signed-off-by: Dimid Duchovny <dimidd@gmail.com>
@sebbASF
Copy link
Contributor

sebbASF commented May 23, 2018

There is now a config module:

https://github.com/apache/incubator-ponymail/blob/master/tools/ponymailconfig.py

This has been used in some scripts.

@sebbASF
Copy link
Contributor

sebbASF commented May 23, 2018

As I wrote previously, I don't think it makes sense to default to using the config file from the current directory. Whilst perhaps convenient for testing, it is confusing to have multiple potential sources.

I think all scripts should use the config in tools/ to ensure consistency.

@dimidd
Copy link
Contributor Author

dimidd commented May 23, 2018

OK, then this PR is redundant.

@dimidd dimidd closed this May 23, 2018
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

3 participants