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

docs on 6.0.1 #270

Merged
merged 1 commit into from
Feb 29, 2024
Merged

docs on 6.0.1 #270

merged 1 commit into from
Feb 29, 2024

Conversation

cel055
Copy link
Contributor

@cel055 cel055 commented Dec 8, 2023 •
edited by jira bot
Loading

@codecov Codecov
Copy link

codecov bot commented Dec 12, 2023 •
edited
Loading

Codecov Report

Merging #270 (0fe7d47) into master (61d135e) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   79.09%   79.07%   -0.02%     
==========================================
  Files          22       22              
  Lines        2057     2060       +3     
==========================================
+ Hits         1627     1629       +2     
- Misses        430      431       +1
Files Coverage Δ
cognite/extractorutils/configtools/__init__.py 100.00% <100.00%> (ø)
cognite/extractorutils/configtools/elements.py 77.71% <ø> (ø)
cognite/extractorutils/configtools/loaders.py 75.00% <ø> (ø)
cognite/extractorutils/exceptions.py 75.00% <ø> (ø)
cognite/extractorutils/util.py 88.34% <ø> (ø)

... and 2 files with indirect coverage changes

@cel055 cel055 force-pushed the docs_6_0_1 branch 2 times, most recently from 4a1d343 to 1ceb452 Compare January 9, 2024 15:00
@cel055 cel055 force-pushed the docs_6_0_1 branch 2 times, most recently from 187527d to 89e1a49 Compare January 25, 2024 09:06
@cel055 cel055 force-pushed the docs_6_0_1 branch 7 times, most recently from a71967c to 6e9bd1a Compare February 12, 2024 09:25
@cel055 cel055 marked this pull request as ready for review February 12, 2024 09:31
@cel055 cel055 requested a review from a team as a code owner February 12, 2024 09:31
mathialo
mathialo reviewed Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@mathialo mathialo left a comment

Choose a reason for hiding this comment

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

Nice, some nitpick

cognite/extractorutils/configtools/elements.py Outdated
@@ -103,6 +112,10 @@ def either_id(self) -> EitherId:


class TimeIntervalConfig(yaml.YAMLObject):
"""
Configuration parameter for setting the time interval
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Configuration parameter for setting the time interval
Configuration parameter for setting a time interval

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/configtools/elements.py Outdated
@@ -167,6 +180,10 @@ def __repr__(self) -> str:


class FileSizeConfig(yaml.YAMLObject):
"""
Configuration parameter for setting the file size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Configuration parameter for setting the file size
Configuration parameter for setting a file size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/configtools/elements.py Outdated
Comment on lines 567 to 568

Valid values: local|remote
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer for these to not be in docs, they are always visible from the code, and we run the risk of them falling out of sync.

Suggested change
Valid values: local|remote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/configtools/elements.py Outdated
database: str
table: str


@dataclass
class RawStateStoreConfig(RawDestinationConfig):
"""
Configuration of the State Store based on CDF RAW
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Configuration of the State Store based on CDF RAW
Configuration of a state store based on CDF RAW

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/configtools/elements.py Outdated
upload_interval: TimeIntervalConfig = TimeIntervalConfig("30s")


@dataclass
class LocalStateStoreConfig:
"""
Configuration of the State Store when using locally
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Configuration of the State Store when using locally
Configuration of a state store using a local JSON file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/util.py Outdated
cognite_client:
heartbeat_waiting_time:
added_message:
extraction_pipeline_ext_id: Id of the extraction pipeline
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
extraction_pipeline_ext_id: Id of the extraction pipeline
extraction_pipeline_ext_id: External ID of the extraction pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/util.py Outdated
heartbeat_waiting_time:
added_message:
extraction_pipeline_ext_id: Id of the extraction pipeline
cognite_client: Assets to create
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cognite_client: Assets to create
cognite_client: Client to use when communicating with CDF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/util.py Outdated
added_message:
extraction_pipeline_ext_id: Id of the extraction pipeline
cognite_client: Assets to create
heartbeat_waiting_time: Interval of the execution of the Thread
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
heartbeat_waiting_time: Interval of the execution of the Thread
heartbeat_waiting_time: Target interval between heartbeats, in seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cognite/extractorutils/util.py Outdated
@add_extraction_pipeline(
extraction_pipeline_ext_id=<INSERT EXTERNAL ID>,
cognite_client=<INSERT COGNITE CLIENT OBJECT>,
logger=<INSERT LOGGER>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this was from the earlier text as well, but it does not seem to me that we expect a logger argument here?

Suggested change
logger=<INSERT LOGGER>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, no there isn't, right now there is only the added message parameter, wich is a str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/source/docs.rst Outdated
Comment on lines 95 to 102
.. ``throttle`` - Tools for throttling
.. -----------------------------------

.. automodule:: cognite.extractorutils.throttle
:members:
:undoc-members:
:inherited-members:
:show-inheritance:
.. .. automodule:: cognite.extractorutils.throttle
.. :members:
.. :undoc-members:
.. :inherited-members:
.. :show-inheritance:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we refactored this module out in a previous release, could you double check that and remove it if we did?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was removed, I left there just so you can see it and maybe tell me something that I don't know. I also left the cognite.extractorutils.uploader_extractor, commented, just so we know that there is something there but it's not documented, do you want me to remove?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cel055 cel055 force-pushed the docs_6_0_1 branch 3 times, most recently from 30d7143 to 58c10b9 Compare February 19, 2024 12:13
@cel055 cel055 requested a review from mathialo February 19, 2024 12:26
@cel055
Docs 02/24
0fe7d47 
[DOG-713]
@cel055 cel055 merged commit f477b37 into master Feb 29, 2024
6 checks passed
@cel055 cel055 deleted the docs_6_0_1 branch February 29, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathialo mathialo mathialo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cel055 @mathialo