You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kibble.apache.org by GitBox <gi...@apache.org> on 2020/10/24 23:11:10 UTC
[GitHub] [kibble] skekre98 opened a new pull request #74: KibbleConfigParser
skekre98 opened a new pull request #74:
URL: https://github.com/apache/kibble/pull/74
I've made `KibbleConfigParser` a subclass of `configparser.ConfigParser`. Currently the class is assuming that the `ini` file has been configured correctly. Let me know if you'd like me to do some error handling if this is not the case.
The usage of this class is as follows:
```python
from parser import KibbleConfigParser
conf = KibbleConfigParser()
for section in conf:
for key in conf[section]:
print(conf[section][key])
# Can also be used as follows
# conf.get(section, key)
```
Output
```bash
True
True
2
0.1.0
kibble
elasticsearch
9200
False
localhost
25
Kibble <no...@kibble.kibble>
```
I have also created a class variable called _uri_ which is in the format _dbname://host:port_. Let me know if this is the format you were looking for.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-717101486
@skekre98 would you mind refactoring `kibble/setup/setup.py` so it uses this config for default values?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716450228
@skekre98 please rebase, you are missing last changes from #66
```
git remote add apache git@github.com:apache/kibble.git
git fetch --all
git rebase apache/master
git push --force
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511526476
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self, ini_file="kibble.ini"):
Review comment:
Instead of default file I would suggest to add:
```
config = KibbleConfigParser()
config.read(path_to_ini_file)
```
This may help in future once we add some tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716075331
Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511525607
##########
File path: kibble/setup/kibble.ini
##########
@@ -0,0 +1,18 @@
+[accounts]
+allowSignup = True
+verify = True
+
+[api]
+database = 2
+version = 0.1.0
+
+[elasticsearch]
+dbname = kibble
+host = elasticsearch
+port = 9200
+ss = False
+
+[mail]
+mailhost = localhost
+mailport = 25
Review comment:
```suggestion
mailhost = localhost:25
```
Probably we can consolidate here too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511569479
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Let's rename this file to `configuration.py`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511526182
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self, ini_file="kibble.ini"):
+ super().__init__()
+ self.read(ini_file)
+
+ # merge elasticsearch url
+ dbname = self["elasticsearch"]['dbname']
+ host = self["elasticsearch"]['host']
+ port = self["elasticsearch"]['port']
+ self.uri = "{}://{}:{}".format(dbname, host, port)
Review comment:
It can also be helpful to add `get_int` and `get_bool` method that would cast the string value of `get` to python type automatically. In example:
https://github.com/apache/airflow/blob/330cd427e1458e92dfe74bd4aefda2392c041282/airflow/configuration.py#L417-L426
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511569266
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self):
+ super().__init__()
+
+ def get_int(self, section, key):
Review comment:
```suggestion
def get_int(self, section: str, key: str) -> int:
```
We don't use mypy yet, but let's add the type annotations
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self):
+ super().__init__()
+
+ def get_int(self, section, key):
+ try:
+ return int(self.get(section, key))
+ except Exception:
+ raise TypeError("Unable to convert value to int")
+
+ def get_bool(self, section, key):
Review comment:
```suggestion
def get_bool(self, section: str, key: str) -> bool:
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 edited a comment on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 edited a comment on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716194751
> > Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`. Any suggestions on how to solve?
>
> Have you done `git add` after running pre-commit?
Yes, I run the commands as follows:
```bash
$ pre-commit run --all-files
$ git add .
$ git commit -m "msg"
$ git push origin master
```
This is the output of the `pre-commit` command:
```bash
(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Fix python encoding pragma...............................................Passed
Add license for all other files..........................................Passed
Add license for all rst files............................................Passed
Add license for all md and html files................(no files to check)Skipped
```
Am I missing something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 edited a comment on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 edited a comment on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716075331
Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`. Any suggestions on how to solve?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 edited a comment on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 edited a comment on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716194751
> > Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`. Any suggestions on how to solve?
>
> Have you done `git add` after running pre-commit?
Yes, I run the commands as follows:
```bash
$ pre-commit run --all-files
$ git add .
$ git commit -m "msg"
$ git push origin master
```
This is the output of the `precommit` command:
```bash
(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Fix python encoding pragma...............................................Passed
Add license for all other files..........................................Passed
Add license for all rst files............................................Passed
Add license for all md and html files................(no files to check)Skipped
```
Am I missing something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716116208
> Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`. Any suggestions on how to solve?
Have you done `git add` after running pre-commit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-717345883
> > @skekre98 would you mind refactoring `kibble/setup/setup.py` so it uses this config for default values?
>
> Could you elaborate on this a bit? Are you looking for me to refactor `get_parser` and return an instance of KibbleConfigParser?
Once we have configuration file for Kibble we should use it in the application:
1. create instance of Kibble config `config = KibbleConfigParser()`
2. use it as `config.get("section", "key")` for default values in https://github.com/apache/kibble/blob/d7f9031dfd93a2efd676fcbd59443feec01df6ed/kibble/setup/setup.py#L43-L54
This will also required some refactor of the setup script + docker-compose to be sure it works.
Let me know if you would like to try do this or should we merge this PR as it is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511530454
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self, ini_file="kibble.ini"):
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-717324034
> @skekre98 would you mind refactoring `kibble/setup/setup.py` so it uses this config for default values?
Could you elaborate on this a bit? Are you looking for me to refactor `get_parser` and return an instance of KibbleConfigParser?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716930980
I apologize for the delay. Still getting used to the repo workflow π
. Let me know if there is anything I left out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511525570
##########
File path: kibble/setup/kibble.ini
##########
@@ -0,0 +1,18 @@
+[accounts]
+allowSignup = True
+verify = True
+
+[api]
+database = 2
+version = 0.1.0
+
+[elasticsearch]
+dbname = kibble
+host = elasticsearch
+port = 9200
Review comment:
```suggestion
conn_uri = elasticsearch:9200
```
Let's consolidate it as mentioned in #52
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511525806
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self, ini_file="kibble.ini"):
+ super().__init__()
+ self.read(ini_file)
+
+ # merge elasticsearch url
+ dbname = self["elasticsearch"]['dbname']
+ host = self["elasticsearch"]['host']
+ port = self["elasticsearch"]['port']
+ self.uri = "{}://{}:{}".format(dbname, host, port)
Review comment:
No need for that, we should be able to access any option using `get` method for example `conf.get("elasticsearch", "host")`.
Let's create config instance that can be accessed by other part of application:
```
conf = KibbleConfigParser()
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511569279
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self):
+ super().__init__()
+
+ def get_int(self, section, key):
+ try:
+ return int(self.get(section, key))
+ except Exception:
+ raise TypeError("Unable to convert value to int")
+
+ def get_bool(self, section, key):
Review comment:
```suggestion
def get_bool(self, section: str, key: str):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 edited a comment on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 edited a comment on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716075331
Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`, any suggestions on how to solve?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716194751
> > Not sure why CI error is persisting, working tree is clean even after `pre-commit run --all-files`. Any suggestions on how to solve?
>
> Have you done `git add` after running pre-commit?
Yes, I run the commands as follows:
```bash
$ pre-commit run --all-files
$ git add .
$ git commit -m "msg"
$ git push origin master
```
This is the output of the `precommit` command:
```bash
(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Fix python encoding pragma...............................................Passed
Add license for all other files..........................................Passed
Add license for all rst files............................................Passed
Add license for all md and html files................(no files to check)Skipped
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek merged pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #74:
URL: https://github.com/apache/kibble/pull/74
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-717353762
Should we perhaps merge this, and address `setup.py` in a different issue? I feel like this may require a bit more understanding from my end which we could discuss on the other issue. I will likely have some elaboration questions when I begin working on it as well βΊοΈ.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] skekre98 commented on pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
skekre98 commented on pull request #74:
URL: https://github.com/apache/kibble/pull/74#issuecomment-716068322
Apologize for the dirty commit log, took me a second to realize `pre-commit` was being used π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511569266
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from configparser import ConfigParser
+
+class KibbleConfigParser(ConfigParser):
+
+ def __init__(self):
+ super().__init__()
+
+ def get_int(self, section, key):
Review comment:
```suggestion
def get_int(self, section: str, key: str):
```
We don't use mypy yet, but let's add the type annotations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kibble] turbaszek commented on a change in pull request #74: KibbleConfigParser
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #74:
URL: https://github.com/apache/kibble/pull/74#discussion_r511569479
##########
File path: kibble/setup/parser.py
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Let's rename this file to `configuration.py` and move it to `kibble/configuration.py`. I don't think we will need the `setup` directory once we have decent cli π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org