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

Implement documents.create #20

Merged
merged 6 commits into from
Aug 2, 2023
Merged

Implement documents.create #20

merged 6 commits into from
Aug 2, 2023

Conversation

zadjadr
Copy link
Contributor

@zadjadr zadjadr commented Jul 21, 2023

Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Didn't had a look at the code, but only about the CI improvement.

Did you saw #18?

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@zadjadr
Copy link
Contributor Author

zadjadr commented Jul 25, 2023

I implemented documents.update and attachments.create as well & will create the PR, if this one is approved and merged

zadjadr#1
zadjadr#2

Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Code looks good so far 👌 Didn't tested it but should work 😉
Just a few nitpitting questions/comments.

Thanks for the efford and the contribution 👍

documents.go Outdated
// title: The title of the document (optional, set to empty string to omit).
// text: The content text of the document.
// publish: A boolean indicating whether the document should be published.
func (cl *DocumentsClient) Create(collectionID CollectionID, title string, text string, publish bool, parentDocumentId ParentDocumentID, templateId TemplateID, template bool) *DocumentsClientCreate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we already want to go with such a complicated strucutre?
We need it earlier or later anyways, but so far we just went with the basics create collection(name), that's it.
Just wanted to questions this, no changes requested.

Also, is it common to have so many parameters in go (in a single line)? 🤔
If is so, I like the Kotlin code style to have each param in its own line:

func (cl *DocumentsClient) Create(
    collectionID CollectionID, 
    title string,
    text string,
    ....,

But I have no strong opinion on that and don't know whats "the way" to go.
@rsjethani ? 🙂

Copy link
Contributor Author

@zadjadr zadjadr Jul 25, 2023

Choose a reason for hiding this comment

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

Do we already want to go with such a complicated strucutre?

  • We (Sre) have a ticket, which needs us to be able to create a document with these features (only template + templateID where added as an extra, since it would be half implemented anyways)

Multiline parameters:
No Idea, I'd also be in favor of the kotlin way :)
We could probably go with any of these: https://realm.github.io/SwiftLint/multiline_parameters.html

Choose a reason for hiding this comment

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

In Go, there is no discussion. gofmt and you take that. :)

Regarding the signature: It's pretty long indeed, maybe you can set some sensible defaults and override those with something like func (cl *DocumentsClient) SetTemplateId(TemplateID)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zadjadr
Multi-line parameters are usually not preferred, they are usually a code smell hinting that code restructuring can reduce number of parameters required. A very straight forward approach is to only ask for mandatory parameters. In this case you should take only title and collection id as compulsory values. For optional values you can have option methods which configure optional behavior. BTW you cannot have create method on DocumentsClient, you need a new type DocumentsClientCreate to maintain consistency across api usage. Following is how the API call can look like eventually:

client.Documents().Create(title, collection id).Text("doc text").Do(...)

Observe: Text is an example of a option method which allows configuring optional behavior. If we do not want to set text for the document then that call can be omitted.

documents.go Outdated Show resolved Hide resolved
documents.go Outdated Show resolved Hide resolved
StefMa
StefMa previously approved these changes Jul 28, 2023
Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Yep, looks good to me 👍
Nice work. 🚀

@StefMa
Copy link
Collaborator

StefMa commented Jul 28, 2023

@rsjethani please have a final look at this 🙏

@zadjadr
Copy link
Contributor Author

zadjadr commented Aug 1, 2023

I resolved the conflicts with upstream/main

Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Just two smaller comments.
However, I wil going to merge this as we should finally get things done.
Improvements can be still done later 👍
I will create a follow-up issues for those things to be evaluated.

DocumentUrlID string
CollectionID string
DocumentID string
ParentDocumentID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if if we need this as an extra type..
Maybe DocumentId is enough, but naming it parentDocumentId 🤷

CreatedAt time.Time `json:"createdAt"`
CreatedBy interface{} `json:"createdBy"`
UpdatedAt time.Time `json:"updatedAt"`
UpdatedBy interface{} `json:"updatedBy"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could already create a User model 🤷

@StefMa StefMa merged commit 5eb3f46 into ioki-mobility:main Aug 2, 2023
3 checks passed
This was referenced Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants