GO-2746: add export structure #1451
Conversation
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
WalkthroughThis update enhances the export module's functionality by introducing structured file handling through new directory constants and a dynamic file naming system based on smart block types. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Coverage provided by /~https://github.com/seriousben/go-patch-cover-action |
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
tests/integration/export_test.go (1)
87-98: Ensure robustness in nested directory handling.The added code correctly handles nested directories. However, consider adding comments for clarity and improving readability.
for _, entry := range entries { if entry.IsDir() { + // Handle nested directories files, err := os.ReadDir(filepath.Join(exportPath, entry.Name())) require.NoError(t, err) for _, file := range files { if filepath.Ext(file.Name()) == ".pb" { foundPbFiles++ } } } else { if filepath.Ext(entry.Name()) == ".pb" { foundPbFiles++ } } }core/block/export/export.go (2)
58-65: Add comments for new directory constants.Adding comments to explain the purpose of each directory constant will improve code readability and maintainability.
const ( // Temporary file name for backups tempFileName = "temp_anytype_backup" // Directory for space-related files spaceDirectory = "spaces" // Directory for type-related files typesDirectory = "types" // Directory for object-related files objectsDirectory = "objects" // Directory for relation-related files relationsDirectory = "relations" // Directory for relation options-related files relationsOptionsDirectory = "relationsOptions" // Directory for template-related files templatesDirectory = "templates" // Directory for file objects filesObjects = "filesObjects" )
538-553: Ensure all block types are handled inprovideFileDirectory.The following
SmartBlockTypevalues are not handled in theprovideFileDirectorymethod:
SmartBlockTypeAccountOldSmartBlockTypePageSmartBlockTypeProfilePageSmartBlockTypeHomeSmartBlockTypeArchiveSmartBlockTypeWidgetPlease review and update the
provideFileDirectorymethod to handle these additionalSmartBlockTypevalues if necessary.
Analysis chain
Ensure all block types are handled in
provideFileDirectory.The switch-case structure should handle all possible
smartblock.SmartBlockTypevalues. Verify that no block types are missed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all possible `smartblock.SmartBlockType` values are handled in `provideFileDirectory`. # Test: Search for the `smartblock.SmartBlockType` enum definition. Expect: All enum values are handled in the switch-case. rg --type go -A 10 $'type SmartBlockType'Length of output: 2886
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- core/block/export/export.go (3 hunks)
- core/block/export/export_test.go (2 hunks)
- core/block/export/writer.go (1 hunks)
- tests/integration/export_test.go (1 hunks)
Additional comments not posted (2)
core/block/export/export_test.go (1)
501-564: Comprehensive test coverage forprovideFileNamemethod.The new test function
Test_provideFileNameprovides thorough validation for theprovideFileNamemethod. Ensure that all edge cases are covered.core/block/export/export.go (1)
528-537: Ensure all calls toprovideFileNameinclude the new parameter.The method signature has been updated to include
blockType. Verify that all calls to this method have been updated accordingly.Verification successful
All calls to
provideFileNameinclude the new parameter.The verification confirms that the method signature update has been appropriately reflected in all instances where
provideFileNameis called.
core/block/export/export_test.gocore/block/export/export.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `provideFileName` include the new parameter. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 1 $'provideFileName('Length of output: 121
Script:
#!/bin/bash # Description: Verify all calls to `provideFileName` include the new parameter. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 1 'provideFileName\('Length of output: 1950
core/block/export/writer.go
Outdated
| dir := filepath.Dir(filename) | ||
| if dir != "" { | ||
| err = os.MkdirAll(filepath.Join(d.path, dir), 0700) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for directory creation.
The added code correctly checks for a non-empty directory and attempts to create it. However, consider logging the error for better traceability.
if dir != "" {
err = os.MkdirAll(filepath.Join(d.path, dir), 0700)
if err != nil {
+ // Log the error for better traceability
+ fmt.Printf("Failed to create directory %s: %v\n", filepath.Join(d.path, dir), err)
return err
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dir := filepath.Dir(filename) | |
| if dir != "" { | |
| err = os.MkdirAll(filepath.Join(d.path, dir), 0700) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| dir := filepath.Dir(filename) | |
| if dir != "" { | |
| err = os.MkdirAll(filepath.Join(d.path, dir), 0700) | |
| if err != nil { | |
| // Log the error for better traceability | |
| fmt.Printf("Failed to create directory %s: %v\n", filepath.Join(d.path, dir), err) | |
| return err | |
| } | |
| } |
core/block/export/writer.go
Outdated
| @@ -58,6 +58,13 @@ func (d *dirWriter) Path() string { | |||
| } | |||
|
|
|||
| func (d *dirWriter) WriteFile(filename string, r io.Reader, lastModifiedDate int64) (err error) { | |||
| dir := filepath.Dir(filename) | |||
| if dir != "" { | |||
There was a problem hiding this comment.
What is the logic here, dir always returns nonempty string, for example fmt.Println(filepath.Dir("x.go")) == ".", you cannot create "." as a directory, it will return file exists. How does it work?
core/block/export/export.go
Outdated
| filename := docID + conv.Ext() | ||
| func (e *export) provideFileName(docID, spaceId string, conv converter.Converter, st *state.State, blockType smartblock.SmartBlockType) string { | ||
| dir := e.provideFileDirectory(blockType) | ||
| filename := filepath.Join(dir, docID+conv.Ext()) |
There was a problem hiding this comment.
I guess we decided to use Id everywhere instead of ID
| if spaceId == "" { | ||
| spaceId := pbtypes.GetString(st.LocalDetails(), bundle.RelationKeySpaceId.String()) | ||
| filename = filepath.Join(spaceDirectory, spaceId, filename) | ||
| } | ||
| return filename | ||
| } | ||
|
|
||
| func (e *export) provideFileDirectory(blockType smartblock.SmartBlockType) string { |
There was a problem hiding this comment.
call it just fileDirectory maybe? and then fileName?
| filename := docID + conv.Ext() | ||
| func (e *export) provideFileName(docID, spaceId string, conv converter.Converter, st *state.State, blockType smartblock.SmartBlockType) string { | ||
| dir := e.provideFileDirectory(blockType) | ||
| filename := filepath.Join(dir, docID+conv.Ext()) | ||
| if spaceId == "" { | ||
| spaceId := pbtypes.GetString(st.LocalDetails(), bundle.RelationKeySpaceId.String()) |
There was a problem hiding this comment.
Can it be the case that spaceId is nil? What happens here, why if spaceId == "" we do something completely different? Maybe It shouldn't be inside this function? Can you explain please?
There was a problem hiding this comment.
It can be empty, if user want to export all spaces. We build file directory based on this value, so I guess it makes sense to check it here 🧐
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/block/export/export.go (4 hunks)
- core/block/export/writer.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- core/block/export/export.go
- core/block/export/writer.go
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/block/export/export.go (5 hunks)
- core/block/export/export_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/block/export/export_test.go
Additional comments not posted (6)
core/block/export/export.go (6)
470-470: Parameter naming change is consistent.The change from
docIDtodocIdaligns with typical Go naming conventions.
529-531: Directory determination logic is efficient.The use of
provideFileDirectoryto determine the directory based onblockTypeis efficient and improves file organization.
539-554: Switch-case structure for directory paths is comprehensive.The
provideFileDirectoryfunction uses a switch-case structure to determine directory paths based onblockType. The cases seem comprehensive and well-structured.
Line range hint
516-524:
Markdown name generation logic is consistent.The logic for generating markdown file names is consistent with the overall file naming strategy.
58-65: Ensure naming consistency for constants.The naming of constants is consistent with the purpose of categorizing exported files. However, consider aligning with existing naming conventions if any discrepancies exist.
Verification successful
Constants are consistently named and used appropriately.
The constants for directory paths are used consistently across the codebase, particularly in constructing file paths and in test assertions. Their naming is clear and descriptive, aligning well with their intended purpose. No discrepancies were found.
- Files Involved:
core/block/export/export.gocore/block/export/export_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and consistency of newly introduced constants in the codebase. # Test: Search for the usage of each constant. Expect: Consistent naming and usage across the codebase. rg --type go 'tempFileName|spaceDirectory|typesDirectory|objectsDirectory|relationsDirectory|relationsOptionsDirectory|templatesDirectory|filesObjects'Length of output: 2225
Line range hint
289-297:
Object retrieval logic is sound.The logic for retrieving objects by IDs, including nested and file options, appears to be sound.
Consider verifying edge cases where
includeNestedandincludeFilesmight affect performance or correctness.
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/block/export/export.go (4 hunks)
Additional comments not posted (5)
core/block/export/export.go (5)
58-65: Review the addition of directory constants.The addition of directory constants (
typesDirectory,objectsDirectory, etc.) improves the organization of exported files. These constants provide a clear and maintainable way to manage directory paths.
Line range hint
470-506:
LGTM! Consistent naming and improved filename generation.The changes in parameter naming and the use of
makeFileNameenhance consistency and maintainability.
516-524: LGTM! Improved readability with parameter renaming.The parameter renaming in
makeMarkdownNameenhances readability and aligns with Go naming conventions.
530-533: LGTM! Enhanced file organization with block type.The use of
blockTypeto determine directory paths improves file organization and flexibility in the export functionality.
541-556: LGTM! Improved modularity withprovideFileDirectory.The
provideFileDirectoryfunction centralizes directory path logic, enhancing modularity and maintainability.
https://linear.app/anytype/issue/GO-2746/export-structure