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

cEP-0026.md: Adds optimize caching cEP #146

Merged
merged 1 commit into from
May 26, 2018
Merged

cEP-0026.md: Adds optimize caching cEP #146

merged 1 commit into from
May 26, 2018

Conversation

palash25
Copy link
Member

No description provided.

@jayvdb
Copy link
Member

jayvdb commented May 11, 2018

Needs rebase, and needs syntax fixup, otherwise your PR will be drowned in gitmate error comments.

cEP-0026.md Outdated
@@ -0,0 +1,188 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpn6adnkb3/cEP-0026.md
+++ b/tmp/tmpn6adnkb3/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata |                                                  |
-| ---------| ------------------------------------------------ |
-| cEP      | 26                                               |
-| Version  | 1.0                                              |
-| Title    | Optimize caching for the NextGen-Core            |
-| Authors  | Palash Nigam <[email protected]>               |
-| Status   | Implementation Due                               |
-| Type     | Feature                                          |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated
## Implementation

The implementation will be divided into three parts:
1. Prototype for `FileFactory` class implementation.

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpn6adnkb3/cEP-0026.md
+++ b/tmp/tmpn6adnkb3/cEP-0026.md
@@ -33,10 +33,10 @@
 ## Implementation
 
 The implementation will be divided into three parts:
-1. Prototype for `FileFactory` class implementation.
-2. Prototype for `Directory` class implementation.
-3. Ignore directories functionality.
-4. Cache control flags.
+1\. Prototype for `FileFactory` class implementation.
+2\. Prototype for `Directory` class implementation.
+3\. Ignore directories functionality.
+4\. Cache control flags.
 
 ### Prototype of FileFactory class implementation
 

cEP-0026.md Outdated
@@ -0,0 +1,188 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpth88cx6d/cEP-0026.md
+++ b/tmp/tmpth88cx6d/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata |                                       |
-| ---------| ------------------------------------- |
-| cEP      | 26                                    |
-| Version  | 1.0                                   |
-| Title    | Optimize caching for the NextGen-Core |
-| Authors  | Palash Nigam <[email protected]>    |
-| Status   | Implementation Due                    |
-| Type     | Feature                               |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated

## Implementation

The implementation will be divided into three parts:

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpth88cx6d/cEP-0026.md
+++ b/tmp/tmpth88cx6d/cEP-0026.md
@@ -33,6 +33,7 @@
 ## Implementation
 
 The implementation will be divided into three parts:
+
 - Prototype for `FileFactory` class implementation.
 - Prototype for `Directory` class implementation.
 - Ignore directories functionality.

cEP-0026.md Outdated
@@ -0,0 +1,189 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpcoso1g9x/cEP-0026.md
+++ b/tmp/tmpcoso1g9x/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata |                                            |
-| ---------| ------------------------------------------ |
-| cEP      | 26                                         |
-| Version  | 1.0                                        |
-| Title    | Optimize caching for the NextGen-Core      |
-| Authors  | Palash Nigam <mailto:[email protected]>  |
-| Status   | Implementation Due                         |
-| Type     | Feature                                    |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated
@@ -0,0 +1,189 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpcoso1g9x/cEP-0026.md
+++ b/tmp/tmpcoso1g9x/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata |                                            |
-| ---------| ------------------------------------------ |
-| cEP      | 26                                         |
-| Version  | 1.0                                        |
-| Title    | Optimize caching for the NextGen-Core      |
-| Authors  | Palash Nigam <mailto:[email protected]>  |
-| Status   | Implementation Due                         |
-| Type     | Feature                                    |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

@palash25 palash25 changed the title cEP-0026.md: Adds optimize caching cEP [WIP] cEP-0026.md: Adds optimize caching cEP May 12, 2018
@palash25 palash25 changed the title [WIP] cEP-0026.md: Adds optimize caching cEP cEP-0026.md: Adds optimize caching cEP May 12, 2018
cEP-0026.md Outdated
performance.

The current caching used by the old core can be improved upon and integrated
with the NextGen-Core by adding a few more enhancements to the current caching
Copy link
Member

Choose a reason for hiding this comment

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

integrated with the NextGen-Core

you can integrate the old stuff into the new one? I'm wondering :D

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not exactly what I meant. What I meant was that we try to take the improvements suggested for the old cache and try to integrate it with the new cache by putting a little spin on it. I can see how that can be confusing.

I now realise that its not important to include that, its confusing. Deleting it.

cEP-0026.md Outdated
to interface with the directories, ignoring unmodified directories during
successive runs and adding cache control flags for better usability and
providing the user various options to perform caching according to a project's
needs.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should mention the already existing caching principle with tasks above in a little paragraph (just some basic explanations), which is fundamental for all new caching features.

cEP-0026.md Outdated

## Implementation

The implementation will be divided into four parts
Copy link
Member

Choose a reason for hiding this comment

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

Add colon ;)

cEP-0026.md Outdated
provide the file contents in different forms for e.g. string, binary
format, etc. The biggest benefit that we will get out of this class would be
that we can replace the actual file contents in our file-dict with these
objects which will itself be a performance improvement.
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see a bit more explanation, especially that file contents are lazy-loaded.

Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't be too hard to also reflect this in the prototype roughly :)

Copy link
Member Author

@palash25 palash25 May 13, 2018

Choose a reason for hiding this comment

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

By lazy-loading you mean read the file in small chunks? Should I add a chunk argument like
file.read(chunk_size) or open(filename, buffering=chunk_size) ?

Copy link
Member

Choose a reason for hiding this comment

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

For example, or reading it only when the user really wants to acces file contents (so not loading it immediately inside __init__)

cEP-0026.md Outdated
self._content = open(self.filename)

def open(filename, newline=None):
return FileFactory(filename, newline=newline)
Copy link
Member

Choose a reason for hiding this comment

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

has to be a classmethod :D

Copy link
Member

Choose a reason for hiding this comment

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

And then ->

@classmethod
def open(cls, ...):
    return cls(...)

cEP-0026.md Outdated
### Prototype of Directory class implementation

The objects of this class will also reside in the file-dict (need to think of
a better name for the file-dict since it will no longer contain any files).
Copy link
Member

Choose a reason for hiding this comment

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

In the end it will become a file-set / directory-set :D

cEP-0026.md Outdated
a better name for the file-dict since it will no longer contain any files).
The objects of this class can also be used by `DirectoryBears` as discussed
on this thread [#4999](https://github.com/coala/coala/issues/4999) by performing
a simple type check on the entries in the dict, whether the objects belong to
Copy link
Member

Choose a reason for hiding this comment

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

belong to -> are instances of

cEP-0026.md Outdated
if os.path.isfile(child):
self.files += 1

self.subdirs = len(self.children) - self.files
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a usecase for count_children?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really its only used to calculate the subdirs. I guess you are right this shouldn't be an attribute of the class but just a temporary variable so I will keep children but remove self from it.

cEP-0026.md Outdated
- `--cache-compression`: This will accept the following arguments: `none`
for compression to be disabled and `lzma` or `gzip` to compress the cache
using these modules.
- `cache-optimization`: For faster cache loading we can make use of this flag
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be called --cache-optimize ;)

cEP-0026.md Outdated
using these modules.
- `cache-optimization`: For faster cache loading we can make use of this flag
which will utilize `pickletools.optimize` for faster cache loading.
- `export-cache` or `import-cache`: To transfer cache across different
Copy link
Member

Choose a reason for hiding this comment

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

I would add "API":

...: API to transfer cache ...

As it's yet to be designed how coala will store the cache ;)

cEP-0026.md Outdated
| cEP | 26 |
| Version | 1.0 |
| Title | Optimize caching for the NextGen-Core |
| Authors | Palash Nigam <mailto:[email protected]> |

Choose a reason for hiding this comment

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

No need to show mailto.
Do something like [mailto](mailto:[email protected])

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to add maito because gitmate suggested it (I just copy pasted the suggested diff). It might give me errors if I try to change it now.

Choose a reason for hiding this comment

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

Can you please try it locally and check if gitmate is throwing errors ?
If so, we can file an issue about.

cEP-0026.md Outdated

This class will be implemented according to the Factory design pattern and will
act as a proxy to dealing with files. This class will have various methods to
provide the file contents in different forms for e.g. string, binary

Choose a reason for hiding this comment

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

in different forms like string, binary...

cEP-0026.md Outdated
This class will be implemented according to the Factory design pattern and will
act as a proxy to dealing with files. This class will have various methods to
provide the file contents in different forms for e.g. string, binary
format, etc. The biggest benefit that we will get out of this class would be

Choose a reason for hiding this comment

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

Please rephrase it as something like:
With the help of this class, we can replace...
I would suggest not to keep too long sentences. You should break it into multiple sentences. Keep it concise and to the point.

cEP-0026.md Outdated

The current caching used by the old core can be improved upon and integrated
with the NextGen-Core by adding a few more enhancements to the current caching
mechanism like `FileFactory` class to load files, `Directory` class

Choose a reason for hiding this comment

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

FileFactory & Directory are not mechanisms. Please rephrase it. 3rd party might not be aware of these classes before reading the details. Its not a good idea to put it in intro.

Choose a reason for hiding this comment

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

You can just say extra classes/method to interface with files/dirs smoothly

cEP-0026.md Outdated

### Ignore directories functionality

The will probably be the biggest performance boost for coala. Instead of only

Choose a reason for hiding this comment

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

Typo in the first word: The -> This

cEP-0026.md Outdated
### Ignore directories functionality

The will probably be the biggest performance boost for coala. Instead of only
ignoring unmodified files on subsequent runs coala shall also ignore directories

Choose a reason for hiding this comment

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

Please add comma after runs

cEP-0026.md Outdated
`FileFactory.parent` and decrease the count of the number of files
(`Directory.files` in the directory-tracker dict) corresponding to that path
by one.
- Once all the files have been deleted from the cache coala will move on to the

Choose a reason for hiding this comment

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

comma after deleted from the cache

cEP-0026.md Outdated

### Cache control flags

This feature enhancement focusses both on performance and usability. The aim

Choose a reason for hiding this comment

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

focusses -> focuses
both on -> on both

cEP-0026.md Outdated
and directories on subsequent coala runs and `lru` (last recently used) in
which cached items only persist until the next run (a count parameter can
also be used to know when to discard an unused cached item).
- `--clear-cache`: Similar to `--flush-cache`.

Choose a reason for hiding this comment

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

Are we providing both --clear-cache & --flush-cache ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure
ping @Makman2

Copy link
Member

Choose a reason for hiding this comment

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

We can to not break API

Choose a reason for hiding this comment

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

Why can't we go ahead with only --flush-cache ? Any particular reason for --clear-cache ?

Copy link
Member

Choose a reason for hiding this comment

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

We even should ;)

cEP-0026.md Outdated
## Documentation

Necessary changes will be made to the NextGen-Core documentation once the
features have been implemented. Besides the NextGen docs a document solely

Choose a reason for hiding this comment

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

comma after NextGen docs

cEP-0026.md Outdated
machines (like the user's local machine and the CI servers) and speed up
builds.

## Documentation
Copy link
Member

Choose a reason for hiding this comment

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

As #123 (comment) suggests, Documentation should better not appear in cEPs.

cEP-0026.md Outdated
def __init__(self, filename, parent):
self.filename = filename # absolute file path
self.parent = os.path.abspath(os.path.join(p, os.pardir)) # parent dir
self._content = open(self.filename)
Copy link
Member Author

@palash25 palash25 May 19, 2018

Choose a reason for hiding this comment

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

I just realised that I have imported linecache but not used it. Should we use it here to access the file contents instead of open do something like

def __init__(self):
    self.lines = linecache.getlines(self.filename)

def file_as_string(self):
    "".join(self.lines)

Copy link
Member

Choose a reason for hiding this comment

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

If you can already provide some prototype, yeah why not :D

Copy link
Member

Choose a reason for hiding this comment

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

But please don't cache the filename itself :P

cEP-0026.md Outdated
return fp.read()

def file_as_string(self):
return self.file_as_bytes().decode()

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Tabs used instead of spaces.

Origin: SpaceConsistencyBear, Section: spacing.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -71,7 +71,7 @@
             return fp.read()
 
     def file_as_string(self):
-      	return self.file_as_bytes().decode()
+        return self.file_as_bytes().decode()
 
     def file_as_list(self):
         self.lines = linecache.getlines(self.filename)

cEP-0026.md Outdated
# Optimize caching for the NextGen-Core

| Metadata | |
| ---------| ----------------------------------------- |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -1,7 +1,7 @@
 # Optimize caching for the NextGen-Core
 
 | Metadata |                                           |
-| ---------| ----------------------------------------- |
+| -------- | ----------------------------------------- |
 | cEP      | 26                                        |
 | Version  | 1.0                                       |
 | Title    | Optimize caching for the NextGen-Core     |

@palash25
Copy link
Member Author

cc @Makman2 @shreyans800755 @fneu please review.

cEP-0026.md Outdated
@@ -0,0 +1,189 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppxdxm84o/cEP-0026.md
+++ b/tmp/tmppxdxm84o/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata  |                                           |
-| --------- | ----------------------------------------- |
-| cEP       | 26                                        |
-| Version   | 1.0                                       |
-| Title     | Optimize caching for the NextGen-Core     |
-| Authors   | Palash Nigam <mailto:[email protected]> |
-| Status    | Implementation Due                        |
-| Type      | Feature                                   |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated
return fp.read()

def file_as_string(self):
return self.file_as_bytes().decode()

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Tabs used instead of spaces.

Origin: SpaceConsistencyBear, Section: spacing.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -71,7 +71,7 @@
             return fp.read()
 
     def file_as_string(self):
-      	return self.file_as_bytes().decode()
+        return self.file_as_bytes().decode()
 
     def file_as_list(self):
         self.lines = linecache.getlines(self.filename)

cEP-0026.md Outdated
# Optimize caching for the NextGen-Core

| Metadata | |
| ---------| ----------------------------------------- |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -1,7 +1,7 @@
 # Optimize caching for the NextGen-Core
 
 | Metadata |                                           |
-| ---------| ----------------------------------------- |
+| -------- | ----------------------------------------- |
 | cEP      | 26                                        |
 | Version  | 1.0                                       |
 | Title    | Optimize caching for the NextGen-Core     |

cEP-0026.md Outdated
@@ -0,0 +1,189 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppxdxm84o/cEP-0026.md
+++ b/tmp/tmppxdxm84o/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata  |                                           |
-| --------- | ----------------------------------------- |
-| cEP       | 26                                        |
-| Version   | 1.0                                       |
-| Title     | Optimize caching for the NextGen-Core     |
-| Authors   | Palash Nigam <mailto:[email protected]> |
-| Status    | Implementation Due                        |
-| Type      | Feature                                   |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated
@@ -0,0 +1,189 @@
# Optimize caching for the NextGen-Core

| Metadata | |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppxdxm84o/cEP-0026.md
+++ b/tmp/tmppxdxm84o/cEP-0026.md
@@ -1,13 +1,13 @@
 # Optimize caching for the NextGen-Core
 
-| Metadata  |                                           |
-| --------- | ----------------------------------------- |
-| cEP       | 26                                        |
-| Version   | 1.0                                       |
-| Title     | Optimize caching for the NextGen-Core     |
-| Authors   | Palash Nigam <mailto:[email protected]> |
-| Status    | Implementation Due                        |
-| Type      | Feature                                   |
+| Metadata |                                           |
+| -------- | ----------------------------------------- |
+| cEP      | 26                                        |
+| Version  | 1.0                                       |
+| Title    | Optimize caching for the NextGen-Core     |
+| Authors  | Palash Nigam <mailto:[email protected]> |
+| Status   | Implementation Due                        |
+| Type     | Feature                                   |
 
 ## Abstract
 

cEP-0026.md Outdated
return fp.read()

def file_as_string(self):
return self.file_as_bytes().decode()

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Tabs used instead of spaces.

Origin: SpaceConsistencyBear, Section: spacing.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -71,7 +71,7 @@
             return fp.read()
 
     def file_as_string(self):
-      	return self.file_as_bytes().decode()
+        return self.file_as_bytes().decode()
 
     def file_as_list(self):
         self.lines = linecache.getlines(self.filename)

cEP-0026.md Outdated
# Optimize caching for the NextGen-Core

| Metadata | |
| ---------| ----------------------------------------- |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -1,7 +1,7 @@
 # Optimize caching for the NextGen-Core
 
 | Metadata |                                           |
-| ---------| ----------------------------------------- |
+| -------- | ----------------------------------------- |
 | cEP      | 26                                        |
 | Version  | 1.0                                       |
 | Title    | Optimize caching for the NextGen-Core     |

cEP-0026.md Outdated
return fp.read()

def file_as_string(self):
return self.file_as_bytes().decode()

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Tabs used instead of spaces.

Origin: SpaceConsistencyBear, Section: spacing.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -71,7 +71,7 @@
             return fp.read()
 
     def file_as_string(self):
-      	return self.file_as_bytes().decode()
+        return self.file_as_bytes().decode()
 
     def file_as_list(self):
         self.lines = linecache.getlines(self.filename)

cEP-0026.md Outdated
# Optimize caching for the NextGen-Core

| Metadata | |
| ---------| ----------------------------------------- |

Choose a reason for hiding this comment

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

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5gqzy4y3/cEP-0026.md
+++ b/tmp/tmp5gqzy4y3/cEP-0026.md
@@ -1,7 +1,7 @@
 # Optimize caching for the NextGen-Core
 
 | Metadata |                                           |
-| ---------| ----------------------------------------- |
+| -------- | ----------------------------------------- |
 | cEP      | 26                                        |
 | Version  | 1.0                                       |
 | Title    | Optimize caching for the NextGen-Core     |

cEP-0026.md Outdated
and directories on subsequent coala runs and `lru` (last recently used) in
which cached items only persist until the next run (a count parameter can
also be used to know when to discard an unused cached item).
- `--clear-cache`: Similar to `--flush-cache`.

Choose a reason for hiding this comment

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

Why can't we go ahead with only --flush-cache ? Any particular reason for --clear-cache ?

cEP-0026.md Outdated
their project's needs. The following cache flags can be implemented to
provide different caching modes:

- `--cache-protocol`: This will accept three different arguments, `none` if

Choose a reason for hiding this comment

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

Can you please mention the default cache protocol if user doesn't specify it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

cEP-0026.md Outdated
This class will follow the Factory design pattern and will act as a proxy to
dealing with files. This class will have various methods to provide the file
contents in different forms like string, binary format, etc. With the help of
this class that we can replace the actual file contents in our file-dict with
Copy link
Member

Choose a reason for hiding this comment

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

that

cEP-0026.md Outdated

@classmethod
def open(cls, filename, newline=None):
return cls(filename, newline=newline)
Copy link
Member

Choose a reason for hiding this comment

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

newline is not present in __init__^^ There's parent instead

cEP-0026.md Outdated

def file_as_list(self):
self.lines = linecache.getlines(self.filename)
self.linecount = len(self.lines)
Copy link
Member

Choose a reason for hiding this comment

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

hm a bit inconsistent that this function suddenly uses linecache while the others don't ;) but not so crucial right now.

cEP-0026.md Outdated
### Prototype of Directory class implementation

The objects of this class will also reside in the file-set (formerly known as
the file-dict) The objects of this class can also be used by `DirectoryBears` as
Copy link
Member

Choose a reason for hiding this comment

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

missing period before "The"

cEP-0026.md Outdated
The objects of this class will also reside in the file-set (formerly known as
the file-dict) The objects of this class can also be used by `DirectoryBears` as
discussed on this thread [#4999](https://github.com/coala/coala/issues/4999) by
performing a simple type check on the entries in the dict, whether the objects
Copy link
Member

Choose a reason for hiding this comment

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

dict --> file-set :)

cEP-0026.md Outdated
- `--cache-protocol`: This will accept three different arguments, `none` if
the user wants to run coala without caching enabled, `primitive` to store
all the cache entries without removing the unchanged ones on subsequent runs
(memory wise quite inefficient but this will prove to be useful in project's
Copy link
Member

Choose a reason for hiding this comment

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

project's --> projects

cEP-0026.md Outdated
the user wants to run coala without caching enabled, `primitive` to store
all the cache entries without removing the unchanged ones on subsequent runs
(memory wise quite inefficient but this will prove to be useful in project's
with small incremental builds that need fast linting), `untrack` the
Copy link
Member

Choose a reason for hiding this comment

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

especially with many incremental builds

Copy link
Member

Choose a reason for hiding this comment

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

You can maybe say "sufficiently small", up to you 👍

cEP-0026.md Outdated
opposite of the `primitive` mode which will stop tracking unchanged files
and directories on subsequent coala runs and `lru` (last recently used) in
which cached items only persist until the next run (a count parameter can
also be used to know when to discard an unused cached item).
Copy link
Member

Choose a reason for hiding this comment

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

Could you make a sublist with bullets for the different caching modes? I mean like

- `none`: blablabla
- `primitive`: blablbla
- ...

That gives a reader a better overview 👍

cEP-0026.md Outdated
- `--cache-compression`: This will accept the following arguments: `none`
for compression to be disabled and `lzma` or `gzip` to compress the cache
using these modules.
- `cache-optimize`: For faster cache loading we can make use of this flag
Copy link
Member

Choose a reason for hiding this comment

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

--> --cache-optimize (dash dash cache optimize 😄 )

`Directory.subdirs` of their parent directory will also be decreased by 1.
- This process will continue for all the directories and the paths having both
the values (`Directory.files` and `Directory.subdirs`) as zero will have
their corresponding `Directory` objects deleted from the cache.
Copy link
Member

Choose a reason for hiding this comment

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

Still a bit vague, but I know this is currently one of our harder parts in this cEP, so we can initially merge it and improve it later on.

cEP-0026.md Outdated

def file_as_list(self):
self.lines = linecache.getlines(self.filename)
self.linecount = len(self.lines)

Choose a reason for hiding this comment

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

Do we really need linecount variable ?
It is implicit (at least right now). I think we can skip it for now, and add in the implementation if it is indeed required.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

cEP-0026.md Outdated

def hash(self):
# hash the absolute file path to get a unique hash
return persistent_hash(self.filename)

Choose a reason for hiding this comment

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

We might wanna store it as data member depending on the complexity of this function call.
Although it can be taken care of while implementing. So no need to change it right now. Just keep this in mind.

cEP-0026.md Outdated
class Directory:
def __init__(self, path):
self.path = path
self.parent = os.path.abspath(os.path.join(p, os.pardir))

Choose a reason for hiding this comment

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

Where is this p coming from in the first param of os.path.join ? Do you mean path ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it should be path. Sorry about that. Will change it.

@shreyans800755
Copy link

ack 7272ff6

@gitmate-bot
Copy link

Sorry @shreyans800755, you do not have the necessary permission levels to perform the action.

@Makman2
Copy link
Member

Makman2 commented May 26, 2018

ack 7272ff6

@Makman2
Copy link
Member

Makman2 commented May 26, 2018

@gitmate-bot ff

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

@gitmate-bot gitmate-bot merged commit 7272ff6 into coala:master May 26, 2018
@gitmate-bot
Copy link

Automated fastforward with GitMate.io was successful! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants